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.

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

Bugfix: ProxyGenerator incompatible with public typed properties and lazy loading #882

Merged
merged 7 commits into from
Jan 10, 2020
Merged

Bugfix: ProxyGenerator incompatible with public typed properties and lazy loading #882

merged 7 commits into from
Jan 10, 2020

Conversation

a-menshchikov
Copy link
Contributor

Added separate methods to get the list of public properties that will be lazy loaded, and to get default values these properties. Typed not nullable properties in php 7.4 have no default value.

Issue: #881

@a-menshchikov
Added separate methods to get the list of public properties that will…
e88621f 
… be lazy loaded, and to get default values these properties (typed not nullable properties in php 7.4 have no default value)
@a-menshchikov
cs fix
9bf79ef
@a-menshchikov
Ignore phpstan error about undefined method ReflectionProperty::getTy…
00e3204 
…pe()
@NikoGrano
Copy link

@Ocramius Would you mind to take a look into this?

alcaeus
alcaeus requested changes Dec 23, 2019
View reviewed changes
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.

Can’t merge this: the new interface methods constitute a BC break. We need to be able to fix this in a minor release.

@a-menshchikov
Copy link
Contributor Author

@alcaeus thank you for comment.
Could you give me some more details about why these new methods constitute a BC break?

@alcaeus
Copy link
Member

alcaeus commented Dec 23, 2019

Adding new methods to an interface is always a BC break, as existing implementations will no longer satisfy the interface and thus cause a fatal error.

@NikoGrano
Copy link

@alcaeus Requesting your review.

NikoGrano
NikoGrano approved these changes Dec 26, 2019
View reviewed changes
Copy link

@NikoGrano NikoGrano left a comment

Choose a reason for hiding this comment

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

LGTM

@a-menshchikov
Copy link
Contributor Author

Added duplicate of ProxyLogicTest that works with class with typed properties. If I have to leave only some specific tests, tell me which ones.

@greg0ire greg0ire requested a review from alcaeus December 31, 2019 16:00
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 2, 2020
@NikoGrano
Still waiting for doctrine/common#882. Temporal dirty quick fix and s…
c402b8c 
…tock inventory implementation.
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 8, 2020
@NikoGrano
Still waiting for doctrine/common#882. Temporal dirty quick fix and s…
1e0628e 
…tock inventory implementation.
@a-menshchikov
Copy link
Contributor Author

@alcaeus what's news on this PR? Maybe I should change something in duplicated tests?

NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 8, 2020
@NikoGrano
Still waiting for doctrine/common#882. Temporal dirty quick fix and s…
d9349fd 
…tock inventory implementation.
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 8, 2020
@NikoGrano
Still waiting for doctrine/common#882. Temporal dirty quick fix and s…
40fcbb1 
…tock inventory implementation.
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 8, 2020
@NikoGrano
Still waiting for doctrine/common#882. Temporal dirty quick fix and s…
2280b08 
…tock inventory implementation.
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 9, 2020
@NikoGrano
Still waiting for doctrine/common#882. Temporal dirty quick fix and s…
a8dcef0 
…tock inventory implementation.
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 9, 2020
@NikoGrano
Still waiting for doctrine/common#882. Temporal dirty quick fix and s…
a0ca17c 
…tock inventory implementation.
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 9, 2020
@NikoGrano
Still waiting for doctrine/common#882. Temporal dirty quick fix and s…
8f29346 
…tock inventory implementation.
@NikoGrano
Copy link

Ping @Ocramius as it's beem 17 days since @alcaeus have commented this.

@alcaeus
Copy link
Member

alcaeus commented Jan 9, 2020

it's beem 17 days since @alcaeus have commented this.

My apologies for spending time with my family over Christmas break. I'm currently working through my notification backlog and have to prioritise things. As this is all done in my free (meaning unpaid) time, sometimes patience is all that's needed.

@NikoGrano
Copy link

Thank you. Just making sure you did not die during x-mass 😆.

Anyways, I understand that you use your free time for this. Just fyi, my current project work done on my free time and not getting paid for it either.

This issue is currently major blocker for anybody using PHP 7.4 in any project and causing everybody to make stypid bypasses or drop version to 7.3. That is the reason why I keep pining. If I can help you in anything, let me know. As I'm private invidual doing this, I'm unfortunately not able sponsor to get this issue resolved any faster.

Ocramius added a commit to Ocramius/common that referenced this pull request Jan 10, 2020
@Ocramius
doctrine#881 doctrine#882 rewrote generated proxies to not use `in_ar…
ec63837 
…ray()` lookups

`in_array()` is expensive, and looking for it on every
property access will cripple performance even without I/O
Ocramius added a commit to Ocramius/common that referenced this pull request Jan 10, 2020
@Ocramius
doctrine#881 doctrine#882 removed reliance on internal property defau…
7eeb0a1 
…lts exposed via public API
Ocramius added a commit to Ocramius/common that referenced this pull request Jan 10, 2020
@Ocramius
doctrine#881 doctrine#882 added more internal type declarations, refa…
37efab2 
…ctored `unset()` codegen
Ocramius added a commit to Ocramius/common that referenced this pull request Jan 10, 2020
@Ocramius
doctrine#881 doctrine#882 using method_exists() rather than `PHP_VE…
9d25b76 
…RSION_ID` comparison

This allows us to use more powerful reflectors (if available).
Ocramius added a commit to Ocramius/common that referenced this pull request Jan 10, 2020
@Ocramius
doctrine#881 doctrine#882 switch testing to 7.4 (stable) rather than …
e764cb5 
…7.4snapshot
Ocramius added a commit to Ocramius/common that referenced this pull request Jan 10, 2020
@Ocramius
doctrine#881 doctrine#882 switch version order to force more CI steps…
2499faa 
… to rely on newer PHP releases
Ocramius added a commit to Ocramius/common that referenced this pull request Jan 10, 2020
@Ocramius
doctrine#881 doctrine#882 removed exclusion rule for `ReflectionPrope…
6f11a7c 
…rty#getType()` (always defined on PHP 7.4)
@Ocramius Ocramius self-assigned this Jan 10, 2020
@Ocramius Ocramius added this to the 2.12.0 milestone Jan 10, 2020
Ocramius
Ocramius approved these changes Jan 10, 2020
View reviewed changes
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM! Applying some local diffs (nitpicks, mostly) and merging manually, thanks @a-menshchikov!

@Ocramius Ocramius merged commit ac7e7f4 into doctrine:master Jan 10, 2020
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 10, 2020
@NikoGrano
Fix typed properties as doctrine/common#882 has been resolved.
6163bf2
@NikoGrano
Copy link

Thanks!

@a-menshchikov a-menshchikov deleted the 881-public-typed branch January 10, 2020 21:05
NikoGrano added a commit to HouseHold/HouseHold that referenced this pull request Jan 17, 2020
@NikoGrano
Fix typed properties as doctrine/common#882 has been resolved.
bcbbbdd
@greedyivan greedyivan mentioned this pull request Feb 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ocramius Ocramius Ocramius approved these changes

@NikoGrano NikoGrano NikoGrano approved these changes

@alcaeus alcaeus Awaiting requested review from alcaeus

Assignees

@Ocramius Ocramius

Labels
New Feature
Projects
None yet
Milestone
2.12.0
Development

Successfully merging this pull request may close these issues.
4 participants
@a-menshchikov @NikoGrano @alcaeus @Ocramius