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

[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

Merged
nicolas-grekas merged 1 commit intosymfony:masterfromenumag:feature/ds-dump
Feb 22, 2019

Conversation

@enumag
Copy link
Contributor

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

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.

keksa and jelen07 reacted with thumbs up emoji
@enumag
Copy link
ContributorAuthor

enumag commentedFeb 20, 2019
edited
Loading

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
Copy link
ContributorAuthor

Btw. should I add: array return types?

@stof
Copy link
Member

this is a new feature, so it goes in master to me.

Please add a line about it in the changelog of the component.

jvasseur reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to thenext milestoneFeb 20, 2019
@nicolas-grekas
Copy link
Member

Btw. should I add: array return types?

yes please

@enumag
Copy link
ContributorAuthor

Btw. should I add: array return types?

yes please

Note that the array return type is not present in other casters.

Another thing is the$isNested parameter which sounds like a boolean but doesn't have the typehint in other cases.

Also I'm not actually using$stub and$isNested so I could just remove them and it should still work.

@nicolas-grekas
Copy link
Member

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
Copy link
ContributorAuthor

Ok, all done.

@stof
Copy link
Member

I suggesting adding thebool typehint on$isNested too.

@enumag
Copy link
ContributorAuthor

enumag commentedFeb 21, 2019
edited
Loading

@stof According to docs Casters can also take a fifth parameter$filter. Should I add it too?

the docs says this about it:

A bit field of Caster ::EXCLUDE_* constants.

https://symfony.com/doc/current/components/var_dumper/advanced.html#casters

@nicolas-grekas
Copy link
Member

Almost no casters use it so no need.

@nicolas-grekas
Copy link
Member

Thank you@enumag.

@nicolas-grekasnicolas-grekas merged commiteab631f intosymfony:masterFeb 22, 2019
nicolas-grekas added a commit that referenced this pull requestFeb 22, 2019
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
Copy link
ContributorAuthor

enumag commentedFeb 22, 2019
edited
Loading

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 dumpingDs\Map. Bacause Map contains object as keys we callMap::pairs() in the caster to get the keys and values asDs\Pair instances and dump those instead. The problem is that since these pairs are only used when dumping, they get destroyed and their object ids can be reused which causes the bug.

Code to reproduce:

$map = unserialize('C:6:"Ds\Map":58:{i:1;C:6:"Ds\Map":8:{i:1;i:2;}i:2;C:6:"Ds\Map":8:{i:1;i:2;}}');dump($map);

Dumped result:

Map {#96 ▼  +0: Pair {#218 ▼    +key: 1    +value: Map {#94 ▼      +0: Pair {#218} // Reused ID from above, dumping is skipped.      capacity: 8    }  }  +1: Pair {#217 ▶}  capacity: 8}

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
Copy link
Member

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
Copy link
ContributorAuthor

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 byDs\Pairs so it's more clear that the pairs in dump are just representation of the inner structure and not actual objects in the map. Alsovar_dump() also dumpsMap asPair instances so I'd like for SFdump to work the same way. So yeah, arrays can fix the issue but I'd very much prever avoiding that if possible.

@nicolas-grekas
Copy link
Member

From what I understand, a new instance is created all the time, so that the reference id is useless?
We should find a way to hide it then.
Using a stub attribute should allow doing that.

@enumag
Copy link
ContributorAuthor

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
Copy link
Member

See#30349

fabpot added a commit that referenced this pull requestFeb 23, 2019
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)![image](https://user-images.githubusercontent.com/243674/53267273-41260e80-36e3-11e9-8723-a73bf0690a01.png)Commits-------45869ac [VarDumper] fix dumping Ds maps and pairs
fabpot added a commit that referenced this pull requestFeb 23, 2019
…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
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
@enumagenumag mentioned this pull requestMar 16, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

4 participants

@enumag@stof@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp