Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when no post processor is defined #526

Merged
merged 1 commit into from
Nov 12, 2014

Conversation

lolautruche
Copy link
Contributor

Exception:

ContextErrorException: Notice: Undefined index: post_processors in /var/www/foo/vendor/liip/imagine-bundle/Liip/ImagineBundle/Imagine/Filter/FilterManager.php line 144

This happens in eZ Publish, where post processors are not (yet) supported.

Ping @crevillo

@@ -141,6 +141,10 @@ public function apply(BinaryInterface $binary, array $config)
*/
public function applyPostProcessors(BinaryInterface $binary, $config)
{
if (!isset($config['post_processors'])) {
return $binary;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better to properly configure it in Configuration.php or DI Extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will fix this in eZ for sure, in order to support post processors. But this shouldn't break if you don't have any configured.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to fix it in LIipImagineBundle, not your code. The configuration or di extnesion of this bundle. The place where filter manager service is configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, sorry misunderstood. Yeah you're probably right, but this check is mandatory anyway since this method is public and we cannot predict what's in $config. So this is an open door for bugs :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lolautruche public.... okay lets keep it here. Could you add a test for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, give me a few minutes (I opened this PR using web 😄 )

@crevillo
Copy link

this will indeed solve the issue. we could solve it in the eZ side too, but to me this is better place to solve. I guess some other projects using Liip can experiment this issue too...

@lolautruche
Copy link
Contributor Author

@crevillo We need to fix this on the eZ side as well, if we want to support post processors in the config.

@crevillo
Copy link

yes. definitely. and it would be good if we can find a way to addopt possible new features added to Liip without the need to touch eZ code ;)

Exception:

> ContextErrorException: Notice: Undefined index: post_processors in /var/www/foo/vendor/liip/imagine-bundle/Liip/ImagineBundle/Imagine/Filter/FilterManager.php line 144

This happens in eZ Publish, where post processors are not (yet) supported.

Ping @crevillo
@lolautruche
Copy link
Contributor Author

Updated code with a better way to add default value for post_processors key. Also added test (that fails without the patch of course).

makasim added a commit that referenced this pull request Nov 12, 2014
Fix crash when no post processor is defined
@makasim makasim merged commit eacdae2 into liip:master Nov 12, 2014
@makasim
Copy link
Collaborator

makasim commented Nov 12, 2014

Thanks

@makasim
Copy link
Collaborator

makasim commented Nov 12, 2014

@lolautruche lolautruche deleted the patch-1 branch November 12, 2014 10:16
@lolautruche
Copy link
Contributor Author

Thank you so much @makasim !

@crevillo
Copy link

this indeed fix the problem in eZ 5 side too. Even eZ 5 won't support post_processors yet. Thanks @makasim and @lolautruche

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants