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

[GH-9807] Fix: initialize potentially empty collections at the hydration complete #9870

Merged
merged 1 commit into from
Jul 3, 2022
Merged

[GH-9807] Fix: initialize potentially empty collections at the hydration complete #9870

merged 1 commit into from
Jul 3, 2022

Conversation

popov-a-e
Copy link
Contributor

Closes #9807

@popov-a-e
Copy link
Contributor Author

Anybody alive here? 🙂

@greg0ire
Copy link
Member

greg0ire commented Jul 1, 2022

Please improve your commit message according to the contributing guide.

@greg0ire greg0ire added the Bug label Jul 1, 2022
@popov-a-e popov-a-e force-pushed the GH-9807 branch 3 times, most recently from 3d7318b to 7e462d2 Compare July 1, 2022 20:54
@popov-a-e
Copy link
Contributor Author

Thanks for the comments @greg0ire, I've fixed it

greg0ire
greg0ire reviewed Jul 1, 2022
View reviewed changes
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

I forgot to mention that in the contribution guidelines, but can you wrap the body of your commit message (which is great, good job!) at 72 chars?

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php Show resolved Hide resolved
@popov-a-e
Copy link
Contributor Author

I forgot to mention that in the contribution guidelines, but can you wrap the body of your commit message (which is great, good job!) at 72 chars?

Hm, do you mean leaving only the headline? 72 chars is not enough to unfold the reasoning

@greg0ire
Copy link
Member

greg0ire commented Jul 1, 2022

Hm, do you mean leaving only the headline?

No, I mean wrapping as in adding a linebreak when a line is 72 chars long. With vim properly configured, you can do that with gqq

greg0ire
greg0ire previously approved these changes Jul 1, 2022
View reviewed changes
@greg0ire greg0ire added this to the 2.12.4 milestone Jul 1, 2022
@popov-a-e
Copy link
Contributor Author

Got it, thanks for the hint. I misunderstood "wrapping" for "reducing"
I use PHPStorm however. Consider me casual developer for not using vim :D

SenseException
SenseException previously approved these changes Jul 2, 2022
View reviewed changes
beberlei
beberlei reviewed Jul 3, 2022
View reviewed changes
Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

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

Good from my first review

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php Show resolved Hide resolved
@popov-a-e
ObjectHydrator: defer initialization of potentially empty collections
79447cb 
If ObjectHydrator faces an empty row to an uninitialized collection,
it initializes it, to prevent it from querying again (DDC-1526).
However, if that row is the first but not the only in the collection,
the next rows will be ignored, as the collection will be considered
"existing", and "existing" collections are only replaced if REFRESH hint
is present. To prevent it, we defer initialization to the end of the
hydration.

Fixes doctrineGH-9807
@popov-a-e popov-a-e dismissed stale reviews from SenseException and greg0ire via 79447cb July 3, 2022 09:25
@greg0ire greg0ire merged commit 83c1ad2 into doctrine:2.12.x Jul 3, 2022
@greg0ire
Copy link
Member

greg0ire commented Jul 3, 2022

Thanks @popov-a-e !

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

@beberlei beberlei beberlei left review comments

@SenseException SenseException SenseException approved these changes

@greg0ire greg0ire greg0ire left review comments

@derrabus derrabus Awaiting requested review from derrabus

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
2.12.4
Development

Successfully merging this pull request may close these issues.

Collection is hydrated as empty if first row contains nulls from a leftJoin
4 participants
@popov-a-e @greg0ire @beberlei @SenseException