Skip to content
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

[VarExporter] Work around php/php-src#12695 for lazy objects, fixing nullsafe-related behavior #52762

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 27, 2023

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Issues #52753
License MIT

This PR fixes another issue discovered with @arnaud-lb while working on lazy-objects.
The root of the issue is described in php/php-src#12695 and this PR implements a fix based on a suggestion by @iluuu1994

Only ghost objects are affected; proxy objects are OK.

$this->assertSame(4, $instance->publicReadonly);
$this->expectException(\BadMethodCallException::class);
$this->expectExceptionMessage('__isset(public)');
isset($instance->public);
Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 27, 2023

Choose a reason for hiding this comment

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

This test case was asserting a behavior that relied on a work around with buggy side effects.
It's now asserting the correct behavior.
Fixing that incorrect behavior is how I spotted the root issue: other existing test cases that use the nullsafe operator were failing.
This means the work around for php/php-src#12695 implemented here is already covered by tests.

@nicolas-grekas nicolas-grekas merged commit 2a00e60 into symfony:6.3 Nov 28, 2023
6 of 9 checks passed
This was referenced Nov 29, 2023
@nicolas-grekas nicolas-grekas deleted the ve-fix-nullsafe branch November 29, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants