Fixed overloading other twig extensions#2
Conversation
… Other extensions should not be overloaded. Also, checking if twig extension is set, otherwise doing nothig.
|
Why did you wrap it in an 'if twig'? If twig does not exist it will just fail silently. |
|
Not really related, but silex.after should not be set as a string. Silex\SilexEvents::BEFORE should be used instead. |
|
True, if you don use the profiler twig tag, it will fail silently, so the ifset is indeed redundant. I still left it to skip the code block, as well as adding the twig extension if twig is not available anyway. |
|
Thanks Igor i'll change it. |
|
If you do not have the twig service provider registered, registering the profiler service will fail with a InvalidArgumentException, hence the checking for the twig identifier in $app. |
|
Well that's the point. If the user is missing the twig extension he wants to know why the ProfilerServiceProvider is not working. That exception is supposed to be thrown, to tell the user that he needs to add twig. If you don't throw the exception, it will just fail silently and the user will be clueless as to why it's not working. |
|
Fair enough, it's a point of view. I'd rather prefer to fail it silently without breaking the app, but I see what you mean. Would you like me to adapt the PR and also incorporate the event-related change @RafalFilipek? |
|
Yes :) Thanks |
… of a string for the after event
3 participants
Hi
I also fixed the way you add your twig profiler extension to the application. Currently, there's no best practice yet for adding your own twig extensions (@see discussion here: https://github.com/fabpot/Silex/issues/160). I took guidance from the KnpMenu Silex extension (@see https://github.com/knplabs/KnpMenu/blob/master/src/Knp/Menu/Silex/KnpMenuServiceProvider.php) since that seems a reasonable approach.
Also, I added a check whether the Twig provider is registered. This might help with cases when it hasn't been setup yet. In that case, the profiler provider does nothing (bad).