Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[VarDumper] Implement DsCaster#30311
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
1954557 toa1b9634Compareenumag commentedFeb 20, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Not dumping the contents of the data structures might be considered a bug by some people. Personally I'd like to see it in 3.x because our project is using LTS Symfony and not seeing the data is often a problem for my colleagues when debugging. On the other hand it should be possible to only upgrade symfony/var-dumper to 4.x and keep the rest as LTS. Let me know if you want me to target an older branch and I'll rebase it. |
enumag commentedFeb 20, 2019
Btw. should I add |
stof commentedFeb 20, 2019
this is a new feature, so it goes in master to me. Please add a line about it in the changelog of the component. |
nicolas-grekas commentedFeb 20, 2019
yes please |
enumag commentedFeb 20, 2019
Note that the array return type is not present in other casters. Another thing is the Also I'm not actually using |
nicolas-grekas commentedFeb 20, 2019
Most of the existing code date from pre-PHP7, that's the reason. New code can use new language features. Let's keep the extra arguments as adding them later would be a BC break, and also because that's the signature of casters. |
enumag commentedFeb 20, 2019
Ok, all done. |
stof commentedFeb 20, 2019
I suggesting adding the |
enumag commentedFeb 21, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@stof According to docs Casters can also take a fifth parameter the docs says this about it:
https://symfony.com/doc/current/components/var_dumper/advanced.html#casters |
nicolas-grekas commentedFeb 21, 2019
Almost no casters use it so no need. |
nicolas-grekas commentedFeb 22, 2019
Thank you@enumag. |
This PR was merged into the 4.3-dev branch.Discussion----------[VarDumper] Implement DsCaster| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |When dumping the data structures from [Ds extension](http://php.net/manual/en/book.ds.php) Symfony only shows the class name but not the actual data. So in this PR I tried to write a Caster for these data structures.Map can't be simply casted to array because it can contain objects as keys so I dump the pairs instead.Commits-------eab631f [VarDumper] Implement DsCaster
enumag commentedFeb 22, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We've tried to use it and my colleague@keksa found a bug. I identified the underlying cause but I'm not exactly sure how to fix it. The problem is with dumping Code to reproduce: Dumped result: I'm not exactly sure how to fix it. An easy fix would be to hold the pairs instances in some global variable and clean it when the dumping is complete but of course that's not exactly clean. Did you encounter a similar problem in any other casters? |
nicolas-grekas commentedFeb 22, 2019
I think then we should drop the Pair object from the dump and make it a simple array with two virtual keys. Would that work for you? |
enumag commentedFeb 22, 2019
I thought about that. While it would work I think it's a worse solution than dumping pairs. People who work with Ds are aware about Map being represented by |
nicolas-grekas commentedFeb 22, 2019
From what I understand, a new instance is created all the time, so that the reference id is useless? |
enumag commentedFeb 22, 2019
The ID being useless is not really a problem. The problem is that dumper (or at least HTML dumper) doesn't render a dump for an object with the same ID as an object rendered before because it assumes they are the same object which is not true in this case. |
nicolas-grekas commentedFeb 22, 2019
See#30349 |
This PR was merged into the 4.3-dev branch.Discussion----------[VarDumper] fix dumping Ds maps and pairs| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Fixes#30311 (comment)Commits-------45869ac [VarDumper] fix dumping Ds maps and pairs
…annot be reused while cloning (nicolas-grekas)This PR was merged into the 3.4 branch.Discussion----------[VarDumper] Keep a ref to objects to ensure their handle cannot be reused while cloning| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Fixes the root issue that led to#30311 (comment)Commits-------29a0683 [VarDumper] Keep a ref to objects to ensure their handle cannot be reused while cloning
When dumping the data structures fromDs extension Symfony only shows the class name but not the actual data. So in this PR I tried to write a Caster for these data structures.
Map can't be simply casted to array because it can contain objects as keys so I dump the pairs instead.