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

[Catalog Promotion] Move catalog promotion processing after the fixture execution #13579

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

lchrusciel
Copy link
Member

@lchrusciel lchrusciel commented Feb 1, 2022

Q A
Branch? 1.11
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

Previous:
With clearing entity manger we don't need to trace that many objects in UnitOfWork, therefore we are not calculating that much changes and we save ton of memory.

Ref. https://blackfire.io/profiles/compare/7c9cb9fd-c43f-4420-8e0a-0af2a7b4b000/graph

Now:
Just a refactor of CP fixtures. The EntityManager will be cleared with https://github.com/symfony/doctrine-bridge/blob/5.4/Messenger/DoctrineClearEntityManagerWorkerSubscriber.php just by the fact, that it will be processed asynced. Which will be done in #13578.

#13578 vs previous implementation -https://blackfire.io/profiles/compare/f560f723-f970-41b3-ac1a-7fe3a8858d77/graph

@lchrusciel lchrusciel requested a review from a team as a code owner February 1, 2022 15:27
@lchrusciel lchrusciel added Feature New feature proposals. Performance labels Feb 1, 2022
@stloyd
Copy link
Contributor

stloyd commented Feb 2, 2022

This looks dangerous for me cause cleaning whole entity manager means that you remove everything that was put in it, either it was already saved or not into database, which means side effects in live applications can be unpredictable IMHO.

@vvasiloi
Copy link
Contributor

vvasiloi commented Feb 2, 2022

I agree with @stloyd. Usually it's flush then clear, doing just a clear is not a good idea.

@lchrusciel lchrusciel changed the title [Catalog Promotion] Clear entity manager for each ApplyCatalogPromotion iteration [Catalog Promotion] Move catalog promotion processing after the fixture execution Feb 2, 2022
@GSadee GSadee merged commit 0dbb05b into Sylius:1.11 Feb 3, 2022
@GSadee
Copy link
Member

GSadee commented Feb 3, 2022

Thanks, Łukasz! 🎉

@lchrusciel lchrusciel deleted the cp-clear-wrapper branch February 3, 2022 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature proposals. Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants