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

add check for uninitialized property, and force reflection if type is… #1375

Closed
wants to merge 2 commits into from

Conversation

re2bit
Copy link

@re2bit re2bit commented Dec 6, 2021

Draft for Discussion of #1367

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? yes
Deprecations? no
Tests pass? ?
Fixed tickets #1367
License MIT

@tmarsteel
Copy link

Awesome, this looks good 👍 I suspected it would have been more work.
Thanks :)

Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

Hello @re2bit!
Thank you for your work! :) It looks good, I left few comments.

In general as forcing Reflection Access might slow down application I would try to limit possible cases when possible - I found that with PHP8.0 has also ReflectionProperty::hasDefaultValue() method that might help to avoid forcing usage of reflection to minimum - what do you think about it?

Best!

$this->forceReflectionAccess = $class->isInternal()
|| $reflectionProperty->isStatic();
$this->hasType = PHP_VERSION_ID >= 70400
&& method_exists($reflectionProperty, 'hasType')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need additional check if method exists? according to docs it should be available since PHP 7.4 - so I would remove additional checks.

$ref = $this->propertyReflectionCache[$metadata->class][$metadata->name] ?? null;
if (null === $ref) {
$ref = new \ReflectionProperty($metadata->class, $metadata->name);
$ref->setAccessible(true);
$this->propertyReflectionCache[$metadata->class][$metadata->name] = $ref;
}

if (PHP_VERSION_ID >= 70400 && !$ref->isInitialized($object)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be executed also for the cases when $metadata->forceReflectionAccess is set to true, but $context->getShouldSerializeUnitializedAsNull() will return false?

@Tobion
Copy link
Contributor

Tobion commented Dec 7, 2021

Introducing setShouldSerializeUnitializedAsNull for this is not good IMO. It makes it impossible to serialize null but not serialize unitialized properties. This is something people will need as that is the whole point of differentiating between unitialized and null.

@re2bit
Copy link
Author

re2bit commented Feb 26, 2024

I am closing this PR due to inactivity.

@re2bit re2bit closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants