Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

Uncaught errors: When templating can't be loaded properly #73

Open
daum opened this issue Sep 18, 2017 · 6 comments
Open

Uncaught errors: When templating can't be loaded properly #73

daum opened this issue Sep 18, 2017 · 6 comments

Comments

@daum
Copy link
Contributor

daum commented Sep 18, 2017

On a project recently we were getting times when we wouldn't be notified of errors. After digging into it a bit more it appears if the templating service has an error, then this bundle won't be notified of the error.

I believe the issue is that the constructor takes in the the templating engine. In our situation (FOS User bundle is installed) when the DB is down twig also fails. This is due to Twig's security extension which in turn creates the UserManager in FOSUserBundle (https://github.com/FriendsOfSymfony/FOSUserBundle/blob/v1.3.7/Doctrine/UserManager.php#L40). Calling getRepository has Doctrine try to get some meta information about the repository. This then triggers an exception as the DB is down/rejecting connections.

As this has occurred as the templating service itself is created, the ErrorNotifierBundle is never able to get it's error hooks OR to handle the kernel exception/console exception events.

I know this is a very specific use case, and on a version of FOS that is a little older, however it does highlight that this bundle has a hard dependency on the Templating which isn't completely necessary to send an error alert like the mailer dependency.

What do you think about making the templating library optional. This way if an error occurs in creation of the templating service the bundle still can handle the error. I'm not sure the best way to remove this dependency as injecting a reference to the container is generally frowned upon, however I think in this case it may be warranted. Just going with the assumption that is how it is handled we could then do the following on https://github.com/Elao/ErrorNotifierBundle/blob/master/Listener/Notifier.php#L333 . Instead of just trying it without a Try/Catch wrap that in it's own try catch and call from the container the templating service. If if fails, send out an email with just the error message and perhaps a miniaturized stack trace?

@benji07
Copy link
Member

benji07 commented Oct 3, 2017

@daum the Templating is a hard dependency because we render the mail using twig

@daum
Copy link
Contributor Author

daum commented Oct 3, 2017

@benji07 Yep totally understand, which is why I was suggesting either making the rendering portion a try/catch and on failure it renders a much simpler email or something similar.

@chalasr
Copy link
Contributor

chalasr commented Oct 3, 2017

@daum Can you paste a full stack trace for the error that prevents rendering?

@daum
Copy link
Contributor Author

daum commented Oct 3, 2017

Yep will try to get it for you by end of week. It's not technically the rendering but the construction of the Twig service in this case with the specific FOSUser bundle version and integration with Twig's security extension.

@chalasr
Copy link
Contributor

chalasr commented Oct 10, 2017

In this case adding a try-catch is not likely to help as the templating->render() line would not be reached. Unless I'm mistaken, the proper fix would be to make this dependency really soft using on-invalid="null" in the listener service definition, updating the constructor arg accordingly and making use of a plaintext view for the notification mail.

Note that this should probably be fixed on FOSUB side instead.

@daum
Copy link
Contributor Author

daum commented Oct 10, 2017

It's actually not an issue in later renditions of FOSUserBundle. It just happened to be the one that led me to figuring out why we weren't getting notifications.

Overall I think the bigger question is requiring that twig and it's related extensions all are able to load without error ok? As I mentioned I know this is a edge case, but I could see someone's custom twig extension causing a 500 error (ie can't connect to a DB on initialization) which being able to still receive error notifications would be beneficial.

As you pointed out a try/catch around the render is not sufficient. We'd need to lazily call the templating engine and wrap that + the render in a try/catch.

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

No branches or pull requests

3 participants