Error logging

[trsteel88]

Currently, if you request a filter that doesn't exist, an exception is thrown. Now, if you have errors automatically being emailed, this can create quite a flood.

This is especially bad if someone is flooding you maliciously with fake filters. Below is an example email we receive on error:

[2015-03-26 12:48:36] request.INFO: Matched route "liip_imagine_filter" (parameters: "_controller": "liip_imagine.controller:filterAction", "filter": "290x180", "path": "0/2/8742/69bf43/image.jpg", "_route": "liip_imagine_filter") [] []
[2015-03-26 12:48:36] request.CRITICAL: Uncaught PHP Exception RuntimeException: "Could not find configuration for a filter: 290x180" at /vendor/liip/imagine-bundle/Liip/ImagineBundle/Imagine/Filter/FilterConfiguration.php line 32 {"exception":"[object] (RuntimeException(code: 0): Could not find configuration for a filter: 290x180 at /vendor/liip/imagine-bundle/Liip/ImagineBundle/Imagine/Filter/FilterConfiguration.php:32)"} []

The actual exception occurs here: https://github.com/liip/LiipImagineBundle/blob/master/Imagine/Filter/FilterConfiguration.php#L32

I suggest that instead of throwing \RunTimeException we throw a Liip\ImagineBundle\Exception\RuntimeException which extends \RunTimeException

Then, in the controller we catch \RunTimeException and then do $this->get('logger')->info($e->getMessage()) if the logger service exists.

I am happy to make this change. I just want to see if this is the best approach or if anything has any input.

Thanks.

[makasim]

Hm, not sure we have to do something here, I believe you have to adjust your error reporting system, like cache same errors for one hour and do not send them again, or use sentry or similar service.

There tons of places where exception are thrown and logged in Symfony itself and other libs and your app. And "an attacker" can cook a request which sends exceptions reports to you.

For example we log even 404 errors and this is super easy to get this exception thrown.

[makasim]

The other thing. Filtering by Liip\ImagineBundle\Exception\RuntimeException is not good idea because this type of exception could be thrown from other places and it may be really important to get it, but it will be filtered out.

[trsteel88]

Yes, 404 exceptions are thrown (and usually caught to display a nice layout).

Yes, those 404 exceptions are logged. However, you easily override the monolog handler to ignore certain errors (such as 404, 503 etc). At this stage, you cannot easily ignore that exception. It is a \RunTimeException so if you were to catch in within the entire application, it may result in other issues.

I think you closed this issue with haste. I agree the Exception should still exist (especially when a developer is implementing the imagine_filter method etc) as it means there is something to fix. However, I don't think that it should be thrown when someone arrives at a image resolver URL and the filter is incorrect. This should be caught, logged and thrown as a 404.

Consider SEO, Google is not going to like if they are indexing images, then all of a sudden you change your design and remove filters. Google is now going to receive errors (rather than 404 responses) if it recrawls those images.

[trsteel88]

The other thing. Filtering by Liip\ImagineBundle\Exception\RuntimeException is not good idea because this type of exception could be thrown from other places and it may be really important to get it, but it will be filtered out.

I am not saying to filter that out. It would still be thrown if the imagine_filter caused this issue. However, any requests to the controller shouldn't throw this exception.

[trsteel88]

@makasim, can you at least re-open this issue so other people using this bundle can have a say?

[makasim]

However, any requests to the controller shouldn't throw this exception.

what should be returned as a response in this case?

[tomtomau]

+1

[trsteel88]

@makasim, I have submitted PR. I believe this should throw a 404 as the filter does not exist. Let me know if you have any queries/tweaks regarding the PR.