8000 [Workflow] Add support for storing the marking in a property by lyrixx · Pull Request #50974 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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? 10000 Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 1, 2023
Merged

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

merged 1 commit into from
Aug 1, 2023

Conversation

lyrixx
Copy link
Member
@lyrixx lyrixx commented Jul 14, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets fixes #49778
License MIT
Doc PR

@carsonbot carsonbot added this to the 6.4 milestone Jul 14, 2023
@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 •
edited
Loading

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 lyrixx force-pushed the 8000 workflow-dic branch from 3c91773 to d644e43 Compare July 27, 2023 17:26
@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

@nicolas-grekas nicolas-grekas force-pushed the workflow-dic branch 2 times, most recently from 5daf667 to cd15e71 Compare July 28, 2023 12:58
@lyrixx lyrixx mentioned this pull request Jul 31, 2023
Merged
@fabpot
Copy link
Member
fabpot commented Aug 1, 2023

Thank you @lyrixx.

@fabpot fabpot merged commit 185941e into symfony:6.4 Aug 1, 2023
@lyrixx lyrixx deleted the workflow-dic branch August 1, 2023 07:57
@fabpot fabpot mentioned this pull request Aug 1, 2023
Closed
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbowyer pbowyer pbowyer left review comments

@stof stof stof requested changes

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@Nyholm Nyholm Awaiting requested review from Nyholm

Assignees
No one assigned
Labels
Feature Status: Reviewed Workflow
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

symfony/workflow: custom marking store with access to property or arguments
6 participants
@lyrixx @nicolas-grekas @fabpot @pbowyer @stof @carsonbot
0