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

Make Workflow using PropertyAccess #51822

Closed
garak opened this issue Oct 3, 2023 · 6 comments
Closed

Make Workflow using PropertyAccess #51822

garak opened this issue Oct 3, 2023 · 6 comments

Comments

@garak
Copy link
Contributor

garak commented Oct 3, 2023

Description

The current implementation of MethodMarkingStore is based on the opinionated assumption that an object has getters and setters.
I find it inconsistent with other Symfony components, where PropertyAccess is used.

Example

<?php

class MyObject
{
    public $property;
}

This is an object you can't use with Workflow, since the MethodMarkingStore class is using 'get'.ucfirst($property) and 'set'.ucfirst($property).

@lyrixx
Copy link
Member

lyrixx commented Oct 8, 2023

Hello. This is already solved in the 6.4 branch, isn't?

@garak
Copy link
Contributor Author

garak commented Oct 8, 2023

Hello. This is already solved in the 6.4 branch, isn't?

No, it isn't.

@mdeboer
Copy link
Contributor

mdeboer commented Oct 8, 2023

Wow, I'm shocked. I assumed it would use PropertyAccess because it is used in so many places (and why wouldn't you).

If there is anything I can do to help or want me to make a PR, please let me know.

@lyrixx
Copy link
Member

lyrixx commented Oct 8, 2023

There is not need here for the property access component. The marking store works with method and property. What do you need more?

For your information, when the marking store set the marking is also pass another argument: the context. And this is not possible to do that With the property access component.


FTR I know 6.4 supports property - I did the PR few month ago.

@garak
Copy link
Contributor Author

garak commented Oct 8, 2023

What if you access your property with a method not named "getSomething"?
About the context, it's not taken into account when you use the property, so it doesn't seem a big concern to me.

@lyrixx
Copy link
Member

lyrixx commented Oct 8, 2023

What if you access your property with a method not named "getSomething"?

So you need to change the name in the configuration.

There is absolutely no need to use the property access component here. You want to use it for the sake of using it. It's brings only drawback setMArking($marking, $context) is not supported.

Again the component deals with property or method:

If you have custom needs, I suggest you to implements your own marking store


I'm going to close this issue, because everything works as expected, and your proposal about internal refactoring does not work as explained.

Thanks for the discussion

@lyrixx lyrixx closed this as completed Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants