Fixing issue with removed class Color #458
Conversation
|
I am fine with fixes for #449 but I am not sure whether it good to remove mongo from dev deps. I prefer to keep it |
|
@havvg ping |
|
Issue is, I cannot run the tests without, let alone run composer install. Not sure if it is a good idea to rely on a halfway exotic extension like mongodb. |
|
if you skip it you cannot say for sure you did not broke anything since some part of the logic is not covered by tests. Also other developers would like to disable something else, how could we tell what we should skip and what not, My position: we have to run all tests |
|
Alternative is to skip everything that is exotic, to lower the barrier of entry for new contributors and have a CI environment that unwinds the full truth by running all the tests. |
|
so ci has to be adjusted to run all tests, it means mongo has to be added to requirements somehow |
| @@ -19,7 +18,7 @@ public function __construct(ImagineInterface $imagine) | |||
| */ | |||
| public function load(ImageInterface $image, array $options = array()) | |||
| { | |||
| $background = new Color(isset($options['color']) ? $options['color'] : '#fff'); | |||
| $background = $image->palette()->color(isset($options['color']) ? $options['color'] : '#fff'); | |||
There was a problem hiding this comment.
Could you pass isset($options['transparency']) ? $options['transparency'] : null as a second parameter here?
This would solve #453.
|
Bump |
|
If you revert changes related to mongo (moving mongo from require-dev to suggest) I am fine to merge it. |
|
@lstrojny it would be good if you can address this too https://github.com/liip/LiipImagineBundle/pull/458/files#r14990108. not required though. |
|
Addressed both issues |
Fixing issue with removed class Color
|
thanks |
|
i think this needs an update to the dependencies, as this won't work with Imagine 0.5 anymore I guess. |
Fixes #449. Additionally moved doctrine/mongodb-odm to the suggest part as it depends on ext/mongo which not everyone has installed.