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

[Workflow] Add support for storing the marking in a property #50974

Merged
merged 1 commit into from Aug 1, 2023

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Jul 14, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets fixes #49778
License MIT
Doc PR

@nicolas-grekas
Copy link
Member

It'd be great to not have to configure anything to store using either a property or a method. We're able to do so in the form component. Maybe add an ObjectMarkingStore that relies on property-access? Or if we really want to have standalone logic, implement all this in one single class that can do both?

@lyrixx
Copy link
Member Author

lyrixx commented Jul 16, 2023

It cannot work since we have two things : the marking and a context. The property access component can only work with one value

So we need to recode everything. Add some cache etc. I'm not sure it really worth it. And anyway, we cannot guess the context property so we Will need to have some convention or configuration anyway

@lyrixx lyrixx requested a review from Nyholm July 24, 2023 15:09
@lyrixx lyrixx force-pushed the workflow-dic branch 2 times, most recently from e09cc57 to 97e97ad Compare July 25, 2023 12:55
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 27, 2023

What about something like that (no need for the "type" option when property is set):

marking_store:
    property: foo

Then:

  • we use getFoo() if it exists, and we use $obj->foo otherwise (one of them must be avail).
  • we use setFoo() if it exists, and we use $obj->foo = $state otherwise (one of them must be avail).

If people want to get the context, we offer only one option: implementing the setter. This enforces an API that legitimately correlates state+context.

@lyrixx
Copy link
Member Author

lyrixx commented Jul 27, 2023

It make sens to me 👍🏼 we can check everything during the compilation

@lyrixx
Copy link
Member Author

lyrixx commented Jul 27, 2023

I pushed a whole new implementation. There is no change in the bundle configuration.

The current MethodMarkingStore has been updated to support methods or property

@lyrixx lyrixx force-pushed the workflow-dic branch 2 times, most recently from 155e7fb to 93eeca2 Compare July 27, 2023 18:59
@lyrixx
Copy link
Member Author

lyrixx commented Jul 28, 2023

Thanks for your review. I addressed your comments

@fabpot
Copy link
Member

fabpot commented Aug 1, 2023

Thank you @lyrixx.

@fabpot fabpot merged commit 185941e into symfony:6.4 Aug 1, 2023
6 of 9 checks passed
@lyrixx lyrixx deleted the workflow-dic branch August 1, 2023 07:57
This was referenced Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

symfony/workflow: custom marking store with access to property or arguments
6 participants