[Form][Profiler] Fixes form collector triggering deprecations #23050
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabpot
approved these changes
Jun 3, 2017
|
Thank you @ogizanagi. |
fabpot
added a commit
that referenced
this pull request
Jun 3, 2017
…ons (ogizanagi) This PR was merged into the 3.3 branch. Discussion ---------- [Form][Profiler] Fixes form collector triggering deprecations | Q | A | ------------- | --- | Branch? | 3.3 <!-- see comment below --> | Bug fix? | yes | New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget updating UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | N/A <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A Since 3.3, if you inspect your logs when accessing the form profiler panel, you'll see some of these: ```sh php.INFO: User Deprecated: The Symfony\Bundle\WebProfilerBundle\Twig\WebProfilerExtension::dumpValue() method is deprecated since version 3.2 and will be removed in 4.0. [...] at /src/Symfony/Bundle/WebProfilerBundle/Twig/WebProfilerExtension.php:119 ``` The [WebProfilerExtension](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/WebProfilerBundle/Twig/WebProfilerExtension.php#L73) is still using a `ValueExporter` instance for BC reasons when the $value ins't an instance of `Data` and this BC layer will be removed in 4.0 (so it'll throw an exception/error when trying to use it with something else than a `Data` instance). The issue is since #21638, collectors (including forms one) have been drastically simplified to leverage the "seamless usage of Data clones", which is great!... But there is a slightly different implementation between `Data::seek()` and [`Data::__get()`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/VarDumper/Cloner/Data.php#L123-L130). There are probably good reasons for this, but it prevents from using classic Twig property access when the underlying data may be a scalar (`null`, `false`, ...). I already spot that while working on the [Validator panel](https://github.com/symfony/symfony/pull/22554/files#diff-deac3c5ce4aa87243093dcd6a3f77a56R84). Perhaps there is a better solution, though. Anyway, current `master` is currently broken, as it still tries to use the `ValueExporter`, which is already removed. And removing the BC layer in `WebProfilerExtension` isn't enough for now. It needs this fix. BTW it also fixes rendering of the concerned inlined-dumps: |Before|After| |--|--| |<img width="818" alt="screenshot 2017-06-03 a 13 35 25" src="https://cloud.githubusercontent.com/assets/2211145/26753222/01a692e6-4862-11e7-90d5-9cc9e4832648.PNG">|<img width="817" alt="screenshot 2017-06-03 a 13 35 47" src="https://cloud.githubusercontent.com/assets/2211145/26753224/090d5d6c-4862-11e7-87c1-73d5346f602c.PNG">| Commits ------- 9de898d [Form][Profiler] Fixes form collector triggering deprecations
![https://cloud.githubusercontent.com/assets/2211145/26753222/01a692e6-4862-11e7-90d5-9cc9e4832648.PNG">|<img](https://cloud.githubusercontent.com/assets/2211145/26753222/01a692e6-4862-11e7-90d5-9cc9e4832648.PNG">|<img){kind=link}
![https://cloud.githubusercontent.com/assets/2211145/26753224/090d5d6c-4862-11e7-87c1-73d5346f602c.PNG">|](https://cloud.githubusercontent.com/assets/2211145/26753224/090d5d6c-4862-11e7-87c1-73d5346f602c.PNG">|){kind=link}
Merged
fabpot
added a commit
that referenced
this pull request
Nov 6, 2017
…rt decision.object being null (ogizanagi) This PR was merged into the 3.3 branch. Discussion ---------- [SecurityBundle] Fix the datacollector to properly support decision.object being null | Q | A | ------------- | --- | Branch? | 3.3 <!-- see comment below --> | Bug fix? | yes | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget to update UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | #24804 <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A Similar to #23050, when accessing a `Data` clone property through `__get()` and the value is `null` for instance, you'll really get `null` instead of a `Data` instance. The solution is to use `seek` instead whenever we access and try to use `profiler_dump` on a `Data` property that can be a simple scalar like `null` or `false`. AFAIK, `decision.object` is the only one here. Commits ------- 769a5f2 [SecurityBundle] Fix the datacollector to properly support decision.object being null
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since 3.3, if you inspect your logs when accessing the form profiler panel, you'll see some of these:
The WebProfilerExtension is still using a
ValueExporterinstance for BC reasons when the $value ins't an instance ofDataand this BC layer will be removed in 4.0 (so it'll throw an exception/error when trying to use it with something else than aDatainstance).The issue is since #21638, collectors (including forms one) have been drastically simplified to leverage the "seamless usage of Data clones", which is great!... But there is a slightly different implementation between
Data::seek()andData::__get(). There are probably good reasons for this, but it prevents from using classic Twig property access when the underlying data may be a scalar (null,false, ...).I already spot that while working on the Validator panel. Perhaps there is a better solution, though.
Anyway, current
masteris currently broken, as it still tries to use theValueExporter, which is already removed. And removing the BC layer inWebProfilerExtensionisn't enough for now. It needs this fix.BTW it also fixes rendering of the concerned inlined-dumps: