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

Reset eager loaders before triggering before find. #10868

Merged
merged 1 commit into from Jul 6, 2017
Merged

Conversation

markstory
Copy link
Member

@markstory markstory commented Jul 6, 2017

In #10657 I made a change to reset eager loaders when creating a clone to fix state being shared across query clones. Because of method ordering this resulted in the eager loader being reset after the beforeFind was triggered, which dropped any contained associations added during that event. Re-ordering the method calls allows contained associations added during the beforeFind to be retained in the count() query.

Refs #10813

In #10657 I made a change to reset eager loaders when creating a clone
to fix state being shared across query clones. Because of method
ordering this resulted in the eager loader being reset *after* the
beforeFind was triggered, which dropped any contained associations added
during that event. Re-ordering the method calls allows contained
associations added during the beforeFind to be retained in the count()
query.

Refs #10813
@markstory markstory added the ORM label Jul 6, 2017
@markstory markstory added this to the 3.4.10 milestone Jul 6, 2017
@codecov-io
Copy link

Codecov Report

Merging #10868 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #10868      +/-   ##
============================================
- Coverage     94.92%   94.92%   -0.01%     
  Complexity    12123    12123              
============================================
  Files           422      422              
  Lines         30142    30142              
============================================
- Hits          28613    28612       -1     
- Misses         1529     1530       +1
Impacted Files Coverage Δ Complexity Δ
src/ORM/Query.php 97.31% <100%> (ø) 94 <0> (ø) ⬇️
src/Cache/Engine/FileEngine.php 85.47% <0%> (-1.12%) 72% <0%> (ø)
src/Cache/CacheEngine.php 93.47% <0%> (+2.17%) 19% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02c51aa...2d59a95. Read the comment docs.

@lorenzo
Copy link
Member

lorenzo commented Jul 6, 2017

Order based dependencies are the worst, glad you could find a way to solve the issue :)

@lorenzo lorenzo merged commit 82afbb0 into master Jul 6, 2017
@lorenzo lorenzo deleted the issue-10813 branch July 6, 2017 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants