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

[DependencyInjection] include file and line number in deprecation#24191

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

Merged

Conversation

@xabbuh
Copy link
Member

QA
Branch?3.3
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

In#23108 we removed line numbers from deprecation messages created by the YAML parser because those numbers were quite useless without the file being parsed. I suggest to revert this change and add the file being parsed to the deprecation message.

robfrawley and ro0NL reacted with thumbs up emoji
@xabbuh
Copy link
MemberAuthor

If we agree on this change, I will update the YAML file loaders from the Routing, Serializer, Translation and Validator components accordingly.

@xabbuhxabbuhforce-pushed thedeprecation-file-and-lineno branch from59092e5 to196727aCompareSeptember 13, 2017 19:08
@nicolas-grekas
Copy link
Member

That is very specific and needs cooperation from the outside. I'm not convinced at all this is a good design...

@xabbuh
Copy link
MemberAuthor

For what it's worth: we already follow a similar approach in theYamlLintCommand.

@ro0NL
Copy link
Contributor

For configuration errors; file > stacktrace.

User Deprecated: Duplicate key "initials" detected whilst parsing YAML. Silent handling of duplicate mapping keys in YAML is deprecated since version 3.2 and will throw \Symfony\Component\Yaml\Exception\ParseException in 4.0.

Stacktrace is huge but doesnt help me at all. I end up searching the project forinitials: in yaml files; got a few hits across files, but also in different branches of the same file. Got lucky, first attempt was good :)

@nicolas-grekas
Copy link
Member

So, this looks like a workaround to me.
Instead, I propose to make Yaml aware of the file name.
We can do it with a flag:Yaml::parse($file, Yaml::PARSE_FILE).
Would be more sensible to me. WDYT?

@ro0NL
Copy link
Contributor

PARSE_FILE sounds good. Wouldnt that fix a bunch of exception wrapping in DI also?

In general i like the goal of; deprecation X > click WDT >click file > fix

@xabbuh
Copy link
MemberAuthor

So, this looks like a workaround to me.
Instead, I propose to make Yaml aware of the file name.
We can do it with a flag:Yaml::parse($file, Yaml::PARSE_FILE).
Would be more sensible to me. WDYT?

My intent was to improve the situation in Symfony 3.3 where such a flag could not be added as this would be a new feature. However, I agree that this would be a cleaner solution and thus I opened#24253.

@xabbuhxabbuhforce-pushed thedeprecation-file-and-lineno branch 4 times, most recently from9126a73 to176589eCompareSeptember 19, 2017 13:44
xabbuh added a commit that referenced this pull requestSep 25, 2017
This PR was merged into the 3.4 branch.Discussion----------[Yaml] support parsing files| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#24191 (comment)| License       | MIT| Doc PR        | TODOThis PR adds a new flag `PARSE_FILE` which can be passed to the YAML parser. When given, the input will be interpreted as a filename whose contents will then be parsed. We already supported passing filenames in the past. Back then the features was not deterministic as it just relied on the string being an existing file or not. Now that we are able to control the behaviour of the parser by passing flags this can be done in a much cleaner way and allows to properly deal with errors (i.e. non existent or unreadable files).This change will also allow to improve error/deprecation messages to include the filename being parsed and thus showing more descriptive error messages when people are using the YAML parser with a bunch of files (e.g. as part of the DependencyInjection or Routing component).Commits-------9becb8a [Yaml] support parsing files
fabpot added a commit that referenced this pull requestSep 27, 2017
This PR was squashed before being merged into the 3.3 branch (closes#24244).Discussion----------TwigBundle exception/deprecation tweaks| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes/no| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->- 1st commit) if you view a exception in the profiler, there is no logger available. Making the tab useless, disabled state is now triggered at zero log messages. There's a specialized panel here.- 2nd commit) when an exception occurs this highlights deprecations in the log table outside the profiler with a warning status. This follows the same signal colors in the profiler.- 3rd commit) hide the default inactive tabs from CSS to avoid scrollbar flickering.- 4th commit) favors document.DOMContentLoaded over window.load, we dont want to wait for images to be loadedFurther out-of-scope improvements could be;- From#24191; i think the logs table should show a direct `View file` link for every error/deprecation/red or yellow line in here. Traversing with `Show context` is tedious.  - links to file.php for your trigger_error() calls  - links to config.yml for trigger_error() calls by SF- From#24151; having the same tooling on both sides is nice- Events/Translations logs is noise, we have specialized panels for those. To further reduce the overall page size container logs can be moved away too, linked from Configuration and/or Logs. Also see#23247Commits-------1c595fc [TwigBundle][WebProfilerBundle] Switch to DOMContentLoaded eventea4b096 [WebProfilerBundle] Hide inactive tabs from CSS0c10f97 [TwigBundle] Make deprecations scream in logs03cd9e5 [TwigBundle] Hide logs if unavailable, i.e. webprofiler
symfony-splitter pushed a commit to symfony/web-profiler-bundle that referenced this pull requestSep 27, 2017
This PR was squashed before being merged into the 3.3 branch (closes #24244).Discussion----------TwigBundle exception/deprecation tweaks| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes/no| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->- 1st commit) if you view a exception in the profiler, there is no logger available. Making the tab useless, disabled state is now triggered at zero log messages. There's a specialized panel here.- 2nd commit) when an exception occurs this highlights deprecations in the log table outside the profiler with a warning status. This follows the same signal colors in the profiler.- 3rd commit) hide the default inactive tabs from CSS to avoid scrollbar flickering.- 4th commit) favors document.DOMContentLoaded over window.load, we dont want to wait for images to be loadedFurther out-of-scope improvements could be;- Fromsymfony/symfony#24191; i think the logs table should show a direct `View file` link for every error/deprecation/red or yellow line in here. Traversing with `Show context` is tedious.  - links to file.php for your trigger_error() calls  - links to config.yml for trigger_error() calls by SF- From #24151; having the same tooling on both sides is nice- Events/Translations logs is noise, we have specialized panels for those. To further reduce the overall page size container logs can be moved away too, linked from Configuration and/or Logs. Also see #23247Commits-------1c595fcf48 [TwigBundle][WebProfilerBundle] Switch to DOMContentLoaded eventea4b0966ab [WebProfilerBundle] Hide inactive tabs from CSS0c10f97f98 [TwigBundle] Make deprecations scream in logs03cd9e553b [TwigBundle] Hide logs if unavailable, i.e. webprofiler
symfony-splitter pushed a commit to symfony/twig-bundle that referenced this pull requestSep 27, 2017
This PR was squashed before being merged into the 3.3 branch (closes #24244).Discussion----------TwigBundle exception/deprecation tweaks| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes/no| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->- 1st commit) if you view a exception in the profiler, there is no logger available. Making the tab useless, disabled state is now triggered at zero log messages. There's a specialized panel here.- 2nd commit) when an exception occurs this highlights deprecations in the log table outside the profiler with a warning status. This follows the same signal colors in the profiler.- 3rd commit) hide the default inactive tabs from CSS to avoid scrollbar flickering.- 4th commit) favors document.DOMContentLoaded over window.load, we dont want to wait for images to be loadedFurther out-of-scope improvements could be;- Fromsymfony/symfony#24191; i think the logs table should show a direct `View file` link for every error/deprecation/red or yellow line in here. Traversing with `Show context` is tedious.  - links to file.php for your trigger_error() calls  - links to config.yml for trigger_error() calls by SF- From #24151; having the same tooling on both sides is nice- Events/Translations logs is noise, we have specialized panels for those. To further reduce the overall page size container logs can be moved away too, linked from Configuration and/or Logs. Also see #23247Commits-------1c595fcf48 [TwigBundle][WebProfilerBundle] Switch to DOMContentLoaded eventea4b0966ab [WebProfilerBundle] Hide inactive tabs from CSS0c10f97f98 [TwigBundle] Make deprecations scream in logs03cd9e553b [TwigBundle] Hide logs if unavailable, i.e. webprofiler
@xabbuhxabbuhforce-pushed thedeprecation-file-and-lineno branch 5 times, most recently from0e8936d to87bdcc4CompareSeptember 28, 2017 14:05
@xabbuhxabbuhforce-pushed thedeprecation-file-and-lineno branch from87bdcc4 tocf03552CompareSeptember 28, 2017 14:22
@nicolas-grekas
Copy link
Member

Thank you@xabbuh.

@nicolas-grekasnicolas-grekas merged commitcf03552 intosymfony:3.3Sep 29, 2017
nicolas-grekas added a commit that referenced this pull requestSep 29, 2017
…ecation (xabbuh)This PR was merged into the 3.3 branch.Discussion----------[DependencyInjection] include file and line number in deprecation| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |In#23108 we removed line numbers from deprecation messages created by the YAML parser because those numbers were quite useless without the file being parsed. I suggest to revert this change and add the file being parsed to the deprecation message.Commits-------cf03552 include file and line number in deprecation
@xabbuhxabbuh deleted the deprecation-file-and-lineno branchSeptember 29, 2017 10:23
@fabpotfabpot mentioned this pull requestOct 5, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

5 participants

@xabbuh@nicolas-grekas@ro0NL@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp