Skip to content

Add typed no default reflection property class usage#94

Merged
beberlei merged 1 commit into1.3.xfrom
add_typed_no_default_reflection_property_class_usage
Jan 16, 2020
Merged

Add typed no default reflection property class usage#94
beberlei merged 1 commit into1.3.xfrom
add_typed_no_default_reflection_property_class_usage

Conversation

@beberlei
Copy link
Member

@beberlei beberlei commented Jan 16, 2020

Supersedes #90

This adds a check for PHP 7.4 so that we don't incur the overhead of getDefaultProperties every time.

It also changes the order of the reflection property creations, because RuntimePublicReflectionProperty already works with typed no default properties, and its the more specialized version that is necessary to be used to don't break lazy loading on typed properties.

@sspat Can you check?

Fixes #89

@sspat
Copy link

sspat commented Jan 16, 2020

@beberlei I understand the changes, looks good!
Are you planning to release this for 1.4, or 1.3 will get it too?
Thank you!

@beberlei beberlei changed the base branch from 1.3.x to master January 16, 2020 09:18
@beberlei beberlei force-pushed the add_typed_no_default_reflection_property_class_usage branch from 26533b2 to 1005a73 Compare January 16, 2020 09:25
@beberlei beberlei changed the base branch from master to 1.3.x January 16, 2020 09:33
@beberlei
Copy link
Member Author

@sspat its going into 1.3

"doctrine/collections": "^1.0",
"doctrine/event-manager": "^1.0",
"doctrine/reflection": "^1.0"
"doctrine/reflection": "^1.1"
Copy link
Member

Choose a reason for hiding this comment

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

Noting that this is required for a fix. Since this transitive dependency fixes the issue for ORM, we've decided to bump this in a patch release.

@alcaeus alcaeus added the Bug Something isn't working label Jan 16, 2020
@alcaeus alcaeus added this to the 1.3.6 milestone Jan 16, 2020
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

👍

@greg0ire greg0ire force-pushed the add_typed_no_default_reflection_property_class_usage branch from 7c8edd1 to f584465 Compare January 16, 2020 21:50
@greg0ire
Copy link
Member

I got rid of the merge commit and squashed the "address Andreas review" commit into the first

@beberlei beberlei merged commit 5dd3ac5 into 1.3.x Jan 16, 2020
@beberlei beberlei deleted the add_typed_no_default_reflection_property_class_usage branch January 16, 2020 22:06

public function __construct()
{
$this->supportsTypedPropertiesWorkaround = version_compare((string) phpversion(), '7.4.0') >= 0;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest replacing this property with an inline \PHP_VERSION_ID > 70400 check, which will be resolved at compile time by OPCache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants