Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 29, 2017

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Sep 13, 2017

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.

@xabbuh
Copy link
Member Author

xabbuh commented Sep 13, 2017

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

@xabbuh xabbuh force-pushed the deprecation-file-and-lineno branch from 59092e5 to 196727a Compare September 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
Member Author

xabbuh commented Sep 15, 2017

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

@ro0NL
Copy link
Contributor

ro0NL commented Sep 15, 2017

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 for initials: 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

ro0NL commented Sep 17, 2017

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
Member Author

xabbuh commented Sep 19, 2017

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.

@xabbuh xabbuh force-pushed the deprecation-file-and-lineno branch 4 times, most recently from 9126a73 to 176589e Compare September 19, 2017 13:44
xabbuh added a commit that referenced this pull request Sep 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        | TODO

This 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 request Sep 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 loaded

Further 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 #23247

Commits
-------

1c595fc [TwigBundle][WebProfilerBundle] Switch to DOMContentLoaded event
ea4b096 [WebProfilerBundle] Hide inactive tabs from CSS
0c10f97 [TwigBundle] Make deprecations scream in logs
03cd9e5 [TwigBundle] Hide logs if unavailable, i.e. webprofiler
symfony-splitter pushed a commit to symfony/web-profiler-bundle that referenced this pull request Sep 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 loaded

Further out-of-scope improvements could be;

- From symfony/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 #23247

Commits
-------

1c595fcf48 [TwigBundle][WebProfilerBundle] Switch to DOMContentLoaded event
ea4b0966ab [WebProfilerBundle] Hide inactive tabs from CSS
0c10f97f98 [TwigBundle] Make deprecations scream in logs
03cd9e553b [TwigBundle] Hide logs if unavailable, i.e. webprofiler
symfony-splitter pushed a commit to symfony/twig-bundle that referenced this pull request Sep 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 loaded

Further out-of-scope improvements could be;

- From symfony/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 #23247

Commits
-------

1c595fcf48 [TwigBundle][WebProfilerBundle] Switch to DOMContentLoaded event
ea4b0966ab [WebProfilerBundle] Hide inactive tabs from CSS
0c10f97f98 [TwigBundle] Make deprecations scream in logs
03cd9e553b [TwigBundle] Hide logs if unavailable, i.e. webprofiler
@xabbuh xabbuh force-pushed the deprecation-file-and-lineno branch 5 times, most recently from 0e8936d to 87bdcc4 Compare September 28, 2017 14:05
@xabbuh xabbuh force-pushed the deprecation-file-and-lineno branch from 87bdcc4 to cf03552 Compare September 28, 2017 14:22
@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit cf03552 into symfony:3.3 Sep 29, 2017
nicolas-grekas added a commit that referenced this pull request Sep 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
@xabbuh xabbuh deleted the deprecation-file-and-lineno branch September 29, 2017 10:23
@fabpot fabpot mentioned this pull request Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants