Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Do not throw away indices from Var Exporter#29715
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
OskarStark left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unlike the improvement of the provideExport() method but my feelings about keeping all indices are 👎 Indeed your case looks wrong, but couldn’t we preserve the old behavior and fix your bug too?
nicolas-grekas commentedDec 28, 2018 • 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.
Hi, thanks for the PR, but that's not a bug. Not displayed indices are increasing numbers from the last explicit index. That matches how PHP indices work. |
simPod commentedDec 28, 2018
How do you propose to solve it then? Scanning the whole array and checking whether indices are sequence or not doesn't sound very performance sensitive to me. Especially for larger arrays. |
nicolas-grekas commentedDec 29, 2018
I propose to close as this output is expected. |
simPod commentedDec 29, 2018
For array |
nicolas-grekas commentedDec 29, 2018
To me it is, and the code matches :) |
simPod commentedDec 29, 2018 • 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.
Got it. I'll show you my use case: Consider this array: [20 =>true,21 =>true,5 =>true,3 =>true,80 =>true,81 =>true,82 =>true,120 =>true,150 =>true,1 =>true,9 =>true,10 =>true,60 =>true,61 =>true,62 =>true,8 =>true,50 =>true,51 =>true,100 =>true,] Current version of VarExporter produces this: [20 =>true,true,5 =>true,3 =>true,80 =>true,true,true,120 =>true,150 =>true,1 =>true,9 =>true,true,60 =>true,true,true,8 =>true,50 =>true,true,100 =>true,] And that gives this when php array is hydrated from VarExporter's output: [20 =>true,21 =>true,5 =>true,3 =>true,80 =>true,81 =>true,82 =>true,120 =>true,150 =>true,1 =>true,9 =>true,151 =>true,60 =>true,152 =>true,153 =>true,8 =>true,50 =>true,154 =>true,100 =>true,] You can see it's not the same array as the origin [ 20 => true, 21 => true, 5 => true, 3 => true, 80 => true, 81 => true, 82 => true, 120 => true, 150 => true, 1 => true, 9 => true,- 10 => true,+ 151 => true, 60 => true,- 61 => true,- 62 => true,+ 152 => true,+ 153 => true, 8 => true, 50 => true,- 51 => true,+ 154 => true, 100 => true, ]; |
xabbuh commentedJan 1, 2019
Happy new year everyone! 🎉 IMO we only need to improve the dump to take into account whether or not the currently exported index is greater than the greatest one (see#29741 for my suggestion to fix this). |
This PR was merged into the 4.2 branch.Discussion----------[VarExporter] fix exporting array indexes| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#29715| License | MIT| Doc PR |Commits-------3c936f4 [VarExporter] fix exporting array indexes
The Var Exporter component was throwing away array indices, eg. when exporting array
[1=>1, 2=>2], it exported[1 => 1, 2]. This fixes this faulty behaviour, however, it preserves indices everywhere. That is IMHO the correct way as there's no safe way to detect whether array indices are explicit or just implicit.