feat: isolate flow execution from business process #5744
Conversation
…usiness-process # Conflicts: # src/Core/Content/Flow/Dispatching/FlowDispatcher.php # tests/unit/Core/Content/Flow/Dispatching/FlowDispatcherTest.php
|
#5744 (comment)< 8000 br> I don't really understand what Danger is on about, maybe i put the files in the wrong directory or something? However the tests are there:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #5744 +/- ##
=======================================
Coverage 50.94% 50.94%
=======================================
Files 4754 4757 +3
Lines 161732 161818 +86
Branches 8817 8817
=======================================
+ Hits 82387 82436 +49
- Misses 76824 76861 +37
Partials 2521 2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…on-from-business-process
Signed-off-by: Benedikt Brunner <benedikt.brunner@pickware.de>
Signed-off-by: Benedikt Brunner <benedikt.brunner@pickware.de>
Signed-off-by: Benedikt Brunner <benedikt.brunner@pickware.de>
|
Hi @Benedikt-Brunner , thanks for your big PR 🎉 . before we start with code review, I checked out your changes and talked with our design department about them. We had the following questions:
We are also having some internal discussions about "the error handling and discovery for the merchant" right now, which isn't an easy topic. Maybe it would make sense to leave the "logging" part out of this PR for now. But to at least get somewhat notified you could maybe use the symfony logger for now. Do you have any other ideas?🤔 |
|
Hi @MalteJanz, thanks for the thorough feedback. We would like to follow the proposal of splitting up the PR into two. This one will only contain the technical changes (moving the flow execution), while the follow up PR will contain all UI/Logging changes. This way we can incorporate the ideas of the design team without impeding the important technical changes "under the hood". I have tested the implementation again to check that it still works (regarding your questions), in production builds everything works as expected (i have attached a video showing the functionality below). However i discovered that if symfony is running in However as we do not know how symfony will respond to this or how long a fix will take, we might need to work around the issue in Shopware. Specifically, the https://github.com/shopware/shopware/blob/trunk/src/Core/Framework/Api/EventListener/ResponseExceptionListener.php should not write to the response after the I would be happy to implement a fix, however i wanted to hear the shopware teams opinion on the matter first. VideoScreen.Recording.2025-01-08.at.13.41.44.mov |
|
PS: regarding question 4:
The changes to the flow execution are behind the |
Signed-off-by: Benedikt Brunner <benedikt.brunner@pickware.de>
Signed-off-by: Benedikt Brunner <benedikt.brunner@pickware.de>
|
Hi @MalteJanz, first of all i want to thank the team for being open to exploring the proposed changes even though their main selling point is now void. I have implemented steps 1-3, i hope i got it all relatively correct. Regarding the proposed Github discussion, should i open that or will shopware handle this? |
|
@Benedikt-Brunner If you want you can also start the GH discussion, I guess you will be quicker than me and are already driving this topic 💪 . It should be an RFC to change the default flow execution model for 6.8 and basically summarize all our discussed points:
|
Co-authored-by: Malte Janz <service.malte.j@protonmail.com>
|
@MalteJanz i seem to lack the necessary permissions for creating a discussion 😅. |
|
@Benedikt-Brunner Alright, then I will create one. Let's first try to get this merged 🙂 |
|
@MalteJanz the failed status checks seem unrelated, should i adjust something? |
|
@Benedikt-Brunner no worries, I also think they were unrelated. We had some issues with our pipelines lately + a lot of stuff is getting merged for the major and because this change isn't urgent we might wait until next week, before trying again ✌️ |
|
@MalteJanz i'll put this back in the queue, I think it just failed in between two dependent platform/commercial prs being merged. |
|
@Benedikt-Brunner as promised I created the discussion here: #6750 I hope it's fine that I reused parts of your ADR to kickstart the discussion with some context. Let me know if the discussion description is missing something important that I should edit. Otherwise let's see what feedback we get and thanks again for your effort pushing this topic forward 🚀 🙂 |
|
Hey @MalteJanz, thanks for opening the Discussion 🥳. The description looks very nice overall, however i would like to distinguish the point about the impact on the business process. While the main error, regarding transaction handling, is now no longer relevant, there are still some similar pain points. Specifically, the flow code still runs during the business process allowing unexpected state mutations during business process execution. Further fatal application crashes (while rare, still very possible in PHP) could still crash the application before the business process can be completed. So i would propose removing the dashed out portions of the text and mention both, that the transaction handling is no longer a concern and that there are still similar concerns we want to address with this change. I can provide text snippets if desired 👋 |
Signed-off-by: Benedikt Brunner <benedikt.brunner@pickware.de> Co-authored-by: Malte Janz <service.malte.j@protonmail.com>
1. Why is this change necessary?
Currently when a flow fails for any reason, the business process (request, queue message, etc.) which triggered it will also fail. This is due to the database transaction being marked for rollback only. In the worst case, this can lead to the store checkout failing for customers, which should always be avoided.
2. What does this change do, exactly?
This changes the flow executor to collect all flow triggers which are dispatched during the business process and then run them only after the business process has concluded.
3. Describe each step to reproduce the issue or behaviour.
4. Please link to the relevant issues (if any).
5. Checklist
Feature Flag
These changes are all contained within the
V6.7.0.0-Major-Feature-Flag!