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 Doctrine type information for multiple state marking stores #19718

Merged

Conversation

Tiriel
Copy link
Contributor

@Tiriel Tiriel commented Mar 27, 2024

Just experienced the behavior described inside the tip box (alongside @Spomky ):

When mapping a workflow array-marking store into Doctrine with the type simple_array and it contains only one key-value pair, the data stored is the value only as a string, resulting in a loss of the current place. Workflow checks then raises exception as the current place is interpreted as 0.

This aims to prevent anyone else frommaking the same mistake, following advice from @nicolas-grekas

.. tip::

You should not use the type ``simple_array`` for your marking store.
Inside a multiple state marking store, places are store as keys with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Inside a multiple state marking store, places are store as keys with
Inside a multiple state marking stored, places are store as keys with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this, I'm talking about the marking_store object.
Going to try for another way to say this, like "Inside a multiple-state marking_store" ?

@OskarStark
Copy link
Contributor

Couldn't this be fixed on the code-side @lyrixx ?

This sounds like a bug to me

@lyrixx
Copy link
Member

lyrixx commented Apr 1, 2024

Couldn't this be fixed on the code-side @lyrixx ?

nope, the number of token can be superior to 1

@Tiriel
Copy link
Contributor Author

Tiriel commented Apr 1, 2024

Couldn't this be fixed on the code-side @lyrixx ?

nope, the number of token can be superior to 1

And checking about with Doctrine's simple_array type, it's also its intended behavior. simple_array converts arrays to text using a simple implode, so keys are obviously lost. A small documentation on the issue would still be a good idea IMHO.

@Tiriel
Copy link
Contributor Author

Tiriel commented Apr 3, 2024

Btw, what should I do with the lint error? Remove the import completely?

#[ORM\Column]
private int $id;

// Type declaration is not mandatory and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this contribution, but this comment is a bit confusing to me:

Type declaration is not mandatory ...

But in the paragraph above we're saying that we need to store this in a JSON type of column.

Copy link
Contributor Author

@Tiriel Tiriel Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. I was trying to say that JSON is the default type inferred by Doctrine, and so one should either not declare any type, or explicitly declare Json type, but not the simple_array. How would you phrase that in a simpler way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could remove the comment and just use the JSON type explicitly in the example. Experienced folks can omit the type if they know that Doctrine will guess it for them.

@javiereguiluz javiereguiluz merged commit 657b8f7 into symfony:5.4 Apr 9, 2024
2 of 3 checks passed
@javiereguiluz
Copy link
Member

Thanks Benjamin!

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.

5 participants