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

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

Merged
merged 1 commit into from Sep 29, 2017

Conversation

Projects
None yet
5 participants
@xabbuh
Member

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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 13, 2017

Member

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

Member

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.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 15, 2017

Member

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

Member

nicolas-grekas commented Sep 15, 2017

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

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 15, 2017

Member

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

Member

xabbuh commented Sep 15, 2017

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

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 15, 2017

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 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 :)

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 16, 2017

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?

Member

nicolas-grekas commented Sep 16, 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?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 17, 2017

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

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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 19, 2017

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?

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.

Member

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 added a commit that referenced this pull request Sep 25, 2017

feature #24253 [Yaml] support parsing files (xabbuh)
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

bug #24244 TwigBundle exception/deprecation tweaks (ro0NL)
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

bug #24244 TwigBundle exception/deprecation tweaks (ro0NL)
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

bug #24244 TwigBundle exception/deprecation tweaks (ro0NL)
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
@fabpot

fabpot approved these changes Sep 28, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 29, 2017

Member

Thank you @xabbuh.

Member

nicolas-grekas commented Sep 29, 2017

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit cf03552 into symfony:3.3 Sep 29, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Sep 29, 2017

bug #24191 [DependencyInjection] include file and line number in depr…
…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 xabbuh:deprecation-file-and-lineno branch Sep 29, 2017

@fabpot fabpot referenced this pull request Oct 5, 2017

Merged

Release v3.3.10 #24452

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment