Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@fancyweb
Copy link
Contributor

@fancywebfancyweb commentedApr 20, 2020
edited
Loading

QA
Branch?3.4
Bug fix?no
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

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.

ro0NL and andrew-demb reacted with thumbs up emojiandrew-demb reacted with hooray emoji
@carsonbotcarsonbot added Status: Needs Review DXDX = Developer eXperience (anything that improves the experience of using Symfony) labelsApr 20, 2020
@xabbuhxabbuh added this to the3.4 milestoneApr 20, 2020
@nicolas-grekas
Copy link
Member

as@stof spotted, all these should be moved outside sprintf()

@nicolas-grekas
Copy link
Member

(but this is for master)

@javiereguiluz
Copy link
Member

This is great! Thanks a lot@fancyweb!

I tested it in the Symfony Demo app and the difference is huge:

Before

before

After

after

fancyweb reacted with rocket emoji

@fabpot
Copy link
Member

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.

@fancyweb
Copy link
ContributorAuthor

as@stof spotted, all these should be moved outside sprintf()

You mean concatenating the exception message after the sprintf?sprintf('The file "%s" does not contain valid YAML', $file).': '.$e->getMessage()

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 22, 2020
edited
Loading

@fancyweb yes - we should also fix existing messages.

@fancywebfancywebforce-pushed thedx-yaml-parse-exception-file-loaders branch fromd57b602 tofc6cf3dCompareApril 23, 2020 14:19
@fancyweb
Copy link
ContributorAuthor

I fixed the existing message in the DI YamlFileLoader. Should I apply fabbot exception message formatting here, ie get rid of the: and just concatenate with a space + $e->getMessage()?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 26, 2020
edited
Loading

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.

fancyweb reacted with thumbs up emoji

@javiereguiluz
Copy link
Member

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 👍

fancyweb reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Thank you@fancyweb.

@nicolas-grekasnicolas-grekas merged commit78a7f46 intosymfony:3.4May 4, 2020
@fancywebfancyweb deleted the dx-yaml-parse-exception-file-loaders branchMay 4, 2020 13:38
This was referencedMay 31, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

Assignees

No one assigned

Labels

DXDX = Developer eXperience (anything that improves the experience of using Symfony)RoutingStatus: ReviewedTranslationValidator

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

6 participants

@fancyweb@nicolas-grekas@javiereguiluz@fabpot@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp