Skip to content

fix: should not use flush_after() in FactoryCollection::create() #908

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

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

nikophil
Copy link
Member

@nikophil nikophil commented May 20, 2025

this reproduces both #907 and #911 and fixes them (two issue, same fix) - @phasdev did add a feature flag for flush once, but the bug still remains.

flush_after() was definitely not a good idea in FactoryCollection::create(): "flush once" behavior should not be a hard requirement, and something like findOrCreate() should actually flush.

thanks again to @phasdev, the reproducers were very helpful!

fixes #907 fixes #911

@nikophil nikophil marked this pull request as draft May 20, 2025 06:38
@nikophil nikophil force-pushed the tests/find-or-create-in-create-many branch from 7ce0936 to 395ca01 Compare May 25, 2025 18:23
#[Test]
#[DataProvider('provideCascadeRelationshipsCombinations')]
#[UsingRelationships(Contact::class, ['category'])]
public function ensure_inverse_one_to_many_is_hydrated_before_persisting_in_flush_after(): void
Copy link
Member Author

@nikophil nikophil May 25, 2025

Choose a reason for hiding this comment

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

I'm not 100% sure the name of the test is accurate, this edge case is pretty specific 🙈 (it reproduces #911)

#[Test]
#[DataProvider('provideCascadeRelationshipsCombinations')]
#[UsingRelationships(Contact::class, ['category'])]
public function find_or_create_used_in_many_actually_create_only_one_object(): void
Copy link
Member Author

Choose a reason for hiding this comment

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

reproduces #907

@nikophil nikophil marked this pull request as ready for review May 25, 2025 18:31
@nikophil nikophil requested review from Copilot and kbond and removed request for Copilot May 25, 2025 18:31
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Reproduces and fixes issues #907 and #911 by adjusting flush behavior when using findOrCreate() inside createMany(), replacing flush_after() in core logic and adding robust tests.

  • Added integration tests covering findOrCreate() in bulk creation and ensuring inverse relationships are hydrated before flush.
  • Changed the persistence layer’s save() to accept multiple objects and flush in one operation.
  • Refactored FactoryCollection::create() to use the new multi-object save() instead of the flush_after() helper.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/Integration/ORM/EntityRelationship/EntityFactoryRelationshipTestCase.php New test cases for bulk findOrCreate() and flush_after() behavior
src/Persistence/functions.php Updated save() helper to delegate to the new persistence API
src/Persistence/PersistenceManager.php Expanded save() to accept variadic objects and flush appropriately
src/FactoryCollection.php Removed flush_after() usage in create() in favor of direct saves
Comments suppressed due to low confidence (1)

src/Persistence/functions.php:122

  • Consider adding or updating the docblock above this helper to explain that it now always returns the original object (the underlying persistence manager no longer returns the entity).
function save(object $object): object

@nikophil nikophil changed the title test: add test case using findOrCreate() in createMany() call fix: should not use flush_after() in FactoryCollection::create() May 25, 2025
@nikophil
Copy link
Member Author

nikophil commented May 25, 2025

I don't know what's happening in this PR: seg fault in codecov, paratest which takes forever... 🤷

EDIT : OK I know why, that's because of data providers

@nikophil nikophil force-pushed the tests/find-or-create-in-create-many branch 2 times, most recently from fb5516a to 28d44f6 Compare May 27, 2025 19:18
@nikophil nikophil force-pushed the tests/find-or-create-in-create-many branch from 28d44f6 to 7c8c308 Compare May 27, 2025 20:07
@smnandre
Copy link
Contributor

🌹

@nikophil
Copy link
Member Author

@smnandre does this fixes the problem you told me about?

@smnandre
Copy link
Contributor

Pretty sure, i'll test friday

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Awesome, thanks everyone!

@smnandre be sure to let us know if this doesn't fix your issue.

@smnandre
Copy link
Contributor

I will, trust me 😆

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

Successfully merging this pull request may close these issues.

Broken tests after upgrade to 2.5 with new flush once feature EntityIdentityCollision after upgrade from 2.4 to 2.5
3 participants