-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DX] Show the ParseException message in all YAML file loaders #36501
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
[DX] Show the ParseException message in all YAML file loaders #36501
Conversation
as @stof spotted, all these should be moved outside sprintf() |
(but this is for master) |
This is great! Thanks a lot @fancyweb! I tested it in the Symfony Demo app and the difference is huge: Before After |
Idea: instead of selectively add the previous exception message to the current exception message, what about doing something similar to some Go error handling libraries where the end message of an error is always the concatenation of all error messages. That's something we could do only in the exception page/profiler. |
You mean concatenating the exception message after the sprintf? |
@fancyweb yes - we should also fix existing messages. |
d57b602
to
fc6cf3d
Compare
I fixed the existing message in the DI YamlFileLoader. Should I apply fabbot exception message formatting here, ie get rid of the |
Showing all stacked messages in a more prominent way is absolutely required I agree. The error page currently is not clear enough in this regard (I recently stayed stuck on an exception because I missed the 2nd one which provided the real issue.) Doing both this change and improving the error page looks like what we should do IMHO. The reason is that we don't have full control over all situations where error messages are used; e.g. exceptions in logs, etc. Still for master on my side - at least that's the policy we've had in the past: that's improving something. |
I agree with @nicolas-grekas about the possible improvements in the error page. We'll discuss about this in the coming weeks to have it ready for Symfony 5.2 👍 |
Thank you @fancyweb. |
This PR synchronizes the exception message in the Routing, Validator and Translation YAML file loaders with the DependencyInjection YAML file loader behavior. Adding the ParseException message is a big DX gain because it highlights the problem directly instead of having to scroll down 7 previous exceptions.
I'm targetting 3.4 because DX can be considered as a bug fix AFAIK.