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 a line about the attribute type when persisted. #9156

Open
wants to merge 6 commits into
base: 3.3
from

Conversation

Projects
None yet
8 participants
@Guikingone
Copy link
Contributor

Guikingone commented Jan 28, 2018

By default, the attribute must be an array in order to allow PropertyAccess to update the state and content of this last one, if we need to persist it via Doctrine, the mapping must define an array.

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 18, 2018

@lyrixx could you please review this proposal? Thanks!

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Mar 19, 2018

With a multiple marking store, it's indeed true.
But I'm not very confortable with this doc. The property name is currentPlace:

  1. It's singular, but we says it should be an array
  2. It's not the default name (marking), so it should be configured. Do we really want to overload the "getting started" documentation ?
@Guikingone

This comment has been minimized.

Copy link
Contributor Author

Guikingone commented Mar 22, 2018

@lyrixx In fact, I've noticed this "approach" after trying to persist the attribute, I can understand that it seems strange to do it.

I've don't see that it overload the getting started block ...

Maybe we can rewrite the default name using currentStage or currentPlace and redefine the one which is used here?

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Mar 22, 2018

About the issue:

  • if a MultipleStateMarkingStore is used, the persisted state type should be an array. Or something that could represent many values (json_array, bit fields ...). Usually with doctrine, we use a json or json_array column.
  • if a SingleStateMarkingStore is used, the persisted state type could be a string
  • if something else is used, it's the responsibility of the creator of the "SomeThingElseMarkingStore" to know how to store the data

And juste to make thing really clear, the MarkingStore is not coupled to a Workflow / StateMachine.
A workflow, by nature will require a "MultipleMarkingStore". But if the workflow is "linear", there are not needs for a multiple marking store. But in 99.99% case, it will.
But as a subject managed by a StateMachine can be in only place, a SingleMarkingStore is enough.

@xabbuh xabbuh added this to the 3.4 milestone Apr 20, 2018

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Apr 25, 2018

Sadly this pull request has stalled. Should we reword it, create a different one or close it as "won't merge"? Thanks!

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Apr 25, 2018

This need to be reworded I think ;)

@Guikingone

This comment has been minimized.

Copy link
Contributor Author

Guikingone commented Apr 25, 2018

Yes, I need to have time to work on the reword, I'm gonna take some time this weekend in order to work on this PR.

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Sep 6, 2018

Friendly ping to not forget about this. Thanks!

@Guikingone

This comment has been minimized.

Copy link
Contributor Author

Guikingone commented Sep 11, 2018

@javiereguiluz Yes, need to take time this week to finish this PR.

fix(storage): storage format
Fix on the storage format (probably need to be reworded).
@ndench

ndench approved these changes Oct 29, 2018

Copy link
Contributor

ndench left a comment

Looks good to me! Thanks for clarifying this.

If a ``multiple_state`` is used, the persisted state type should be an ``array``,
using Doctrine, you can use ``json`` or ``json_array``.

If a ``single_state`` is used, the persisted state type could be a string

This comment has been minimized.

Copy link
@noniagriconomie

noniagriconomie Dec 20, 2018

Contributor

Or it could be also an integer, but you need to implement a MarkingStoreInterface to convert from string to int the place.

I see more often values of a $status property of object stored as int in the database, not string
Explaining it here could be very usefull to inform that we can do it easily IMO :)

cc @Guikingone

This comment has been minimized.

Copy link
@Guikingone

Guikingone Dec 24, 2018

Author Contributor

Thanks for the feedback, I've added new lines about this case, can you review it and tell me if it's adapted? 🤔

Thanks 😊

improvement(DB storage): improvement on type stored
An improvement has been made in order to explain the usage of an int via MarkingStoreInterface
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 2, 2019

Should we close this pull request without merging it? First, the mentioned json_array type was deprecated in Doctrine. Second, the edge-case mentioned in single_state looks very edgy to me and I don't think we should mention it. Thoughts? Thanks!

@noniagriconomie

This comment has been minimized.

Copy link
Contributor

noniagriconomie commented Jan 2, 2019

@javiereguiluz

maybe wait symfony/symfony#29146 to be merged? and after document all cases?

but i really think a documentation (with example) is a must have for this part
actually lof of people using the workflow are using it with symfony, and with database storage

having a use case could be a nice one :)

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 2, 2019

No It should not be closed. This PR is good 👍 Let's merge it (except for json_array)

@Guikingone

This comment has been minimized.

Copy link
Contributor Author

Guikingone commented Jan 2, 2019

No It should not be closed. This PR is good 👍 Let's merge it (except for json_array)

We could remove the json_array if it's a problem, not a big deal 😄

If a ``multiple_state`` is used, the persisted state type should be an ``array``,
using Doctrine, you can use ``json`` or ``json_array``.

If a ``single_state`` is used, the persisted state type could be a string,

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jan 2, 2019

Member

"could be a string" or "should be a string" ... we say "should be an array" in the previous paragraph.


If something else is used, you're in charge of choosing the best storage format.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jan 2, 2019

Member

Does the previous tip related to MarkingStoreInterface apply in this case too? If yes, we must merge these two phrases.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Apr 7, 2019

Member

@javiereguiluz yes it's apply to. So it should be merged


.. note::

If a ``multiple_state`` is used, the persisted state type should be an ``array``,

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jan 2, 2019

Member

This prhase needs some reword. It reads as "should be an array using Doctrine". Do you mean, a "PHP array" in the entity property and a "json" type in the @Column mapping?

@@ -138,7 +138,7 @@ like this:
class BlogPost
{
// This property is used by the marking store
// This property is used by the marking store, by default, Symfony "mark" this attribute as an array.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jan 2, 2019

Member

I don't understand what this means --> "Symfony mark this attribute as an array" Is this something that can only be understood if you use the Workflow component (which I don't) ... or is it supposed to be understood by all readers? Thanks!

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jan 2, 2019

Member

May be this is better:

By default symfony set the marking as an array.

as or with, I don't know

Guikingone added some commits Jan 2, 2019

@@ -138,7 +138,7 @@ like this:
class BlogPost
{
// This property is used by the marking store

This comment has been minimized.

Copy link
@noniagriconomie

noniagriconomie Jan 13, 2019

Contributor

Is it the case with state machine as well?
State machine has only one place allowed at the same time

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 28, 2019

@Guikingone please, rebase this to 3.4 so we can merge it. Thank you.

@Guikingone Guikingone force-pushed the Guikingone:patch-8 branch 2 times, most recently from 55b36e1 to d1c0dd0 Apr 3, 2019

@Guikingone

This comment has been minimized.

Copy link
Contributor Author

Guikingone commented Apr 3, 2019

@noniagriconomie @lyrixx

Hi 👋

Can you please check one last time this PR before the final rebase, this way, we can close it, thanks :)

@noniagriconomie

This comment has been minimized.

Copy link
Contributor

noniagriconomie commented Apr 3, 2019

@Guikingone there are some changes/new feature arround this storage part of the workflow
cf symfony/symfony@59f20ad
maybe this PR need to be closed, or adapted

@lyrixx
Copy link
Member

lyrixx left a comment

Thanks for your PR. I added few comments.

@@ -138,7 +138,7 @@ like this:
class BlogPost
{
// This property is used by the marking store
// This property is used by the marking store, by default, Symfony set the marking as an array.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Apr 7, 2019

Member

To make things simple the current object is managed by:
A state machine: It's a string
A workflow: it's an array


If something else is used, you're in charge of choosing the best storage format.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Apr 7, 2019

Member

@javiereguiluz yes it's apply to. So it should be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.