Skip to content

[VarExporter] Missing fix for lazy objects with hook properties #59843

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Rixafy opened this issue Feb 23, 2025 · 8 comments
Closed

[VarExporter] Missing fix for lazy objects with hook properties #59843

Rixafy opened this issue Feb 23, 2025 · 8 comments

Comments

@Rixafy
Copy link

Rixafy commented Feb 23, 2025

Symfony version(s) affected

7.2, 7.3

Description

It appears that fix #59761 was not merged all the way to 7.2 and 7.3, there is a code that is missing. The code should work with variable $hookedProperties, as in 6.4, but in 7.2 and 7.3 the variable is only created and not used later in code.

https://github.com/symfony/symfony/blame/7.3/src/Symfony/Component/VarExporter/Internal/LazyObjectRegistry.php#L69

How to reproduce

Covered in tests of the pull request.

Possible Solution

No response

Additional Context

No response

@xabbuh
Copy link
Member

xabbuh commented Feb 23, 2025

Can you elaborate on the "how to reproduce" part please? Which tests from which PR are you talking about? The tests added in #59761 are present in the 7.3 branch and tests pass.

@Rixafy
Copy link
Author

Rixafy commented Feb 23, 2025

That's surprising, because I stumbled upon an error that was fixed by that PR, but when you look at the code in 7.3
https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/VarExporter/Internal/LazyObjectRegistry.php#L69

that $hookedProperties is not used anywhere, and if you look at blame and original commit in 7.3, it's there 540253a#diff-24aa8979f11c659eb27e40dfe974658b81b97995fcd383674c573184e9aff563R86

But when you look at the current file, it's just not, so I thought this may be the bug I was experiencing, because when I downgraded to 6.4 where that variable is used, it worked. I'm not sure exactly how to reproduce that Cannot unset private(set) property.. error, because it was somewhere in doctrine when I was using private(set) properties (flush).

I'm now using 6.4 and unfortunately I don't have time to isolate it and write tests, it seemed like some merge has overwritten that part of code where array variable $hookedProperties was used, because on first sight it's unused variable.

@fabpot
Copy link
Member

fabpot commented Feb 26, 2025

Closing as it's just that the 6.4 branch has not been merged yet into 7.2 and 7.3.
For your information, branch merges are done manually and so not in real-time.

@fabpot
Copy link
Member

fabpot commented Feb 26, 2025

Sorry, I checked 7.1, not 7.2. Can you confirm that #59860 fixes your issue?

@fabpot fabpot reopened this Feb 26, 2025
@Rixafy
Copy link
Author

Rixafy commented Feb 26, 2025

Thanks, but for some reason it doesn't, that property is defined as private(set) ?string $email = null;, in 6.4 it works. I'll try to find out why maybe in the UTC evening, because there is another bug anyways that will appear after this one is fixed, but both can be now bypassed when I set property to protected, I will create separate issue when I isolate it.

Image

The another one I will report (when I will time to isolate it) is similar (Cannot modify private(set) property) and it's here, maybe it's related, but there is slight chance it may be only doctrine orm related, since it's not ready for hooked properties.

@nicolas-grekas
Copy link
Member

This issue description is super misleading: instead of describing the real issue, it describes a guess of a solution. The useful hint came after, in the error message: "Cannot unset private(set) property". And THIS, leads to the correct understanding: support for asymmetric visibility - not hooks. I guess asymmetric visibility needs another round of fixes.
Now that all this is sorted out, the $hookedProperties property thing has nothing to do with that.
And we can start wondering about the real fix :)

@Rixafy
Copy link
Author

Rixafy commented Feb 27, 2025

Yeah, sorry, it seems that problem will be somewhere else and this was just dead code that didn't have effect when corrected as in 6.4, even if the getClassResetters is 1:1 with 6.4, in 7.3 there is still that error, I tried to isolate it but with no luck, when I'll have more time I will give it anther look and create an issue that can be reproduced.

@nicolas-grekas
Copy link
Member

See #59884 for the fix

nicolas-grekas added a commit that referenced this issue Feb 28, 2025
…as-grekas)

This PR was merged into the 6.4 branch.

Discussion
----------

[VarExporter] Fix support for asymmetric visibility

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #59843
| License       | MIT

Commits
-------

7321bbf [VarExporter] Fix support for asymmetric visibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants