fix: get raw value on uninitialized proxy#12379
Open
gseidel wants to merge 7 commits intodoctrine:3.6.xfrom
Open
fix: get raw value on uninitialized proxy#12379gseidel wants to merge 7 commits intodoctrine:3.6.xfrom
gseidel wants to merge 7 commits intodoctrine:3.6.xfrom
Conversation
Member
|
Can you please add a functional test that reproduces your bug? |
Contributor
Author
|
@derrabus I was struggling to isolate the bug from my application and was only able to reproduce it at on Mac with version 8.4.16. Check #12380. As you can see, it works fine in CI, and it also looks more like a PHP bug. Originally I just pass the page to a Symfony form. On the other hand, |
Member
|
Could it be caused by XDebug? It looks similar to #12378 If yes, maybe we should add a conflict with old versions of XDebug in |
Contributor
Author
|
Yes, I believe this is the same issue, and it's also related to symfony/symfony#61058. The bug in Xdebug makes it possible for objects to remain uninitialized even when they should have been initialized. As a result, an uninitialized proxy could end up reaching the In #12378, @fabianbloching independently arrived at the same fix. |
Member
|
In theory, I would be opposed to merging a fix for an upstream bug. In practice, your patch seems to make the code simpler. However, I'm not sure about the performance implications of this. Also, you're modifying the constructor of a class that is not declared as internal (maybe it should be, but still…). There are existing phpbench tests that might call this piece of code. You can add an exception and run them to see if that's the case. Then, it's possible to save benchmark data and compare the data between your commit and the commit on 3.6.x |
Contributor
Author
|
@greg0ire The constructor is private, so there is no BC-Break here. Benchmark is another topic, let me try to figure out if this change has a performance impact. |
Member
|
Ah right. You can check the phpbench docs to figure this out, otherwise I might provide more help later. |
Member
|
This is the docs I was hinting at: https://phpbench.readthedocs.io/en/latest/guides/regression-testing.html If I throw an Exception in |
Member
|
Here are the results of the benchmark: As you can see, at least 2 benchmarks are negatively impacted: EDIT: I do not reproduce this result reliably 🤦 This is probably fine from the performance standpoint. |
Member
|
Performance-centric discussion about the array cast: doctrine/persistence#307 (comment) |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The
RawValuePropertyAccessorreturnsnullfromgetValueif the accessed object is an uninitialized proxy. This leads to further problems. In my case, theBasicEntityPersister::loadOneToManyCollectionfunction returns wrong objects if the parent is still an uninitialized proxy, because the query'sWHEREclause tries to find rows where the ID isNULL.