Skip to content

[VarExporter] Fix: Use correct closure call for property-specific logic in $notByRef #60258

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 27, 2025
Merged

[VarExporter] Fix: Use correct closure call for property-specific logic in $notByRef #60258

merged 1 commit into from
Apr 27, 2025
+35 −1

Conversation

Hakayashii
Copy link
Contributor

@Hakayashii Hakayashii commented Apr 23, 2025 •
edited by OskarStark
Loading

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

Previously, $notByRef was treated as a callable instead of an array of closures. Now, the closure is correctly invoked with $notByRef[$name]($object, $value) if it's not true.

Encountered the following error after loading an entity from the database using Doctrine, which held a reference to another entity. The error occurred when modifying this lazily-loaded reference.

[critical] Uncaught Error: Array callback must have exactly two elements

In Hydrator.php line 163:
                                                 
  [Error]                                        
  Array callback must have exactly two elements  
  
Exception trace:
  at C:\Users\hakayashii\repos\test\vendor\symfony\var-exporter\Internal\Hydrator.php:163
 Proxies\__CG__\App\Entity\Subscription->{closure:Symfony\Component\VarExporter\Internal\Hydrator::getSimpleHydrator():155}() at C:\Users\hakayashii\repos\test\vendor\symfony\var-exporter\Hydrator.php:72
 Symfony\Component\VarExporter\Hydrator::hydrate() at C:\Users\hakayashii\repos\test\vendor\symfony\var-exporter\Internal\LazyObjectState.php:56
 Symfony\Component\VarExporter\Internal\LazyObjectState->initialize() at C:\Users\hakayashii\repos\test\vendor\symfony\var-exporter\LazyGhostTrait.php:146
 Proxies\__CG__\App\Entity\Subscription->__get() at C:\Users\hakayashii\repos\test\src\Service\OrderBuilder.php:45
[...]

@carsonbot carsonbot added this to the 6.4 milestone Apr 23, 2025
nicolas-grekas
nicolas-grekas reviewed Apr 23, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, my bad.
Do you think you could add a test case to cover this?

@symfony symfony deleted a comment from carsonbot Apr 23, 2025
@symfony symfony deleted a comment from carsonbot Apr 23, 2025
@carsonbot carsonbot changed the title Fix: Use correct closure call for property-specific logic in $notByRef [VarExporter] Fix: Use correct closure call for property-specific logic in $notByRef Apr 24, 2025
nicolas-grekas
nicolas-grekas approved these changes Apr 27, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please account for stof's comment. The fix is good.

@Hakayashii
Copy link
Contributor Author

I added a new test case for property hooks with a default value. Therefore, I had to move the added property from the Hooked class to a separate HookedWithDefaultValue class, because otherwise the existing test case would have failed too.

@fabpot
Copy link
Member

fabpot commented Apr 27, 2025

Thank you @Hakayashii.

@fabpot fabpot merged commit c5d39dd into symfony:6.4 Apr 27, 2025
1 check passed
@xabbuh xabbuh mentioned this pull request Apr 27, 2025
Merged
fabpot added a commit that referenced this pull request May 2, 2025
@fabpot
bug #60288 [VarExporter] dump default value for property hooks if pre…
b5b27a8 
…sent (xabbuh)

This PR was merged into the 6.4 branch.

Discussion
----------

[VarExporter] dump default value for property hooks if present

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

The test added in #60258 reveals that we have another bug in our lazy ghost generation logic which leads to an error at runtime when a hook tries to read the property's default value.

Commits
-------

e819dab dump default value for property hooks if present
This was referenced May 2, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@ging-dev ging-dev ging-dev left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed VarExporter
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
7 participants
@Hakayashii @fabpot @nicolas-grekas @stof @xabbuh @ging-dev @carsonbot