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] Added a context to `Workflow::apply()` #29146

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@lyrixx
Member

lyrixx commented Nov 8, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27925 (maybe #28253 and #28266)
License MIT
Doc PR
@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 8, 2018

I thought it will be easy, but the Property Access does not support passing 2 arguments to a setter (super logic after all).

I don't know how to achieve this. Some randome ideas

  1. Pass a PlaceAndContext to the POPO. but the BC layer will be impossible
  2. do not rely on the the PropertyAccess anymore (but this imply re-code a (small) part of it in the workflow)
  3. Add a MarkingStoreWithContext that does not rely on ProperyAccess and support only setXXXX method call (or with an Interface)
  4. use the PropertyAccess to call setMarking then setContext

Have you other ideas ? What do you prefer ?

Actually I prefer the 2/
It quite easy. we only need to check if a property if public or if method marking() exist, or setMarking() exist. We could still use the PropertyAccessor for getMarking.

@stof

This comment has been minimized.

Member

stof commented Nov 8, 2018

@lyrixx I think you are forgetting the fact that PropertyAccess takes a property path as input, not just a property name. Using PropertyAccess to read but a simpler implementation supporting only property names to write will not work.

@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 8, 2018

Ah you are right. So we should go to 3/

What do you think ?

@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 8, 2018

Just one question, as we do not mention we are using the Property Path component, is it a BC break a change the implementation ?

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 8, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 8, 2018

Not if nothing changes on the outside surface.What would be the user visible side effect?

@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 8, 2018

@nicolas-grekas It could change someting to the end user if they use this kind of configuration:

framework:
    workflows:
        my_workflow:
            marking_store:
                type: multiple_state
                arguments: 
                    - 'property1.property2'
  1. the arguments is documented there : https://symfony.com/doc/current/workflow/usage.html#creating-a-workflow
  2. We don't explain that property1.property2 is supported.

So, if no one is using this feature (we can not be sure, I know, but really I doubt someone is using it), there are no difference for the end user

@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 8, 2018

I case of you think it's a BC break, what do you think if the last commit?

@noniagriconomie

This comment has been minimized.

Contributor

noniagriconomie commented Nov 9, 2018

@lyrixx thank you on coding this 👍

just a question: what is the aim behind MethodSingleStateMarkingStore? not sure to understand (i mainly use this component with single state)

also, how can we deal with sort of "validation" of the context?
optionresolver? interface? i think it could be safer to have this kind of control

@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 12, 2018

The aim of MethodSingleStateMarkingStore is to be able to call $subject->setMarking($place, $context).

also, how can we deal with sort of "validation" of the context?

We will not validate the context. It's not the role of the workflow component to do that.

@noniagriconomie

This comment has been minimized.

Contributor

noniagriconomie commented Nov 12, 2018

fair enough :) 👍

@lyrixx

This comment has been minimized.

Member

lyrixx commented Nov 13, 2018

Here we go. I think this PR is ready.

Here is the diff the developer will have to do to use this new feature:
lyrixx/SFLive-Paris2016-Workflow#21

@noniagriconomie

thank you for the feature

@@ -36,8 +36,7 @@ public function getMarking($subject);
/**
* Sets a Marking to a subject.
*
* @param object $subject A subject
* @param Marking $marking A marking
* @param object $subject A subject

This comment has been minimized.

@noniagriconomie

noniagriconomie Nov 14, 2018

Contributor

need a doc update, no?

This comment has been minimized.

@lyrixx

lyrixx Nov 14, 2018

Member

Why? What do you suggest?

* @param string $property Used to determine methods to call
* The `getMarking` method will use `$subject->getProperty()`
* The `setMarking` method will use `$subject->setProperty(string|array $places, array $context = array())`
*/

This comment has been minimized.

@noniagriconomie

noniagriconomie Nov 14, 2018

Contributor

missing @param bool for singleState

This comment has been minimized.

@lyrixx

lyrixx Nov 14, 2018

Member

No need. We don't duplicate PHP doc when the parameter has a type hint and is obvious

*/
public function getMarking($subject)
{
$marking = $subject->{'get'.ucfirst($this->property)}();

This comment has been minimized.

@noniagriconomie

noniagriconomie Nov 14, 2018

Contributor

what if method does not exist in the subject due to fail syntax?

This comment has been minimized.

@lyrixx

lyrixx Nov 14, 2018

Member

It will lead to a fatal error, and so an exception.

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