8000 Various Workflow classes are declared final without any apparent reason. · Issue #36358 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

Various Workflow classes are declared final without any apparent reason. #36358

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

Jump to bottom
Closed
ghost opened this issue Apr 6, 2020 · 12 comments
Closed

Various Workflow classes are declared final without any apparent reason. #36358

ghost opened this issue Apr 6, 2020 · 12 comments
Labels
Bug Status: Needs Review Workflow

Comments

@ghost
Copy link
ghost commented Apr 6, 2020

Symfony version(s) affected: 5.0.7

Description

Various classes in symfony/workflow are decleared final, preventing extension. For example I cannot extend Symfony\Component\Workflow\MarkingStore\MethodMarkingStore to account for 0 values which are valid in my case.

How to reproduce

Try extending Symfony\Component\Workflow\MarkingStore\MethodMarkingStore

Possible Solution

Remove final declaration from class.

Additional context
@ghost ghost added the Bug label Apr 6, 2020
@lmasforne
Copy link
Contributor

Hello @efthymiou ,
Can you explain more with objects exemples in what case you dont need to define values in this class ?
Thanks

@ghost
Copy link
Author
ghost commented Apr 7, 2020 
class Value {
    /** @var int this may be 0 */
   protected $marking;
  
   // …
}

Now the provided MethodMarkingStore has the following check in the getMarking() method

    public function getMarking(object $subject): Marking
    {
        // …
        if (!$marking) {
            return new Marking();
        }

That does not work in my case since 0, which is a valid value in my case, evaluates to false.

What is the rationale behind the MethodMarkingStore class being final? Why is it prohibited to extend it?

The InMemoryMetadataStore class is also declared as final. Why?

@nicolas-grekas
Copy link
Member

Final classes help maintainance without removing any possibilities from users, usually. We need to first confirm your case cannot be addressed in any other way than inheritance before taking the hassle of increased maintenance overhead.

@ghost
Copy link
Author
ghost commented Apr 7, 2020

Well I obviously can copy'n'paste the provided code and change the getMarking function accordingly.

@Devristo
Copy link
Contributor
Devristo commented Apr 7, 2020

Wouldn't a custom marking store suffice in this case?

framework:
    workflows:
        my_workflow:
            marking_store:
                service: 'App\Service\MyMarkingStore'

@ghost
Copy link
Author
ghost commented Apr 7, 2020 •
edited by ghost
Loading

I do not use the yaml configuration, but yes as I said, I can make a custom MarkingStore by copy'n'pasting the code in MethodMarkingStore into MyMarkingStore

This was referenced Apr 7, 2020
Merged
Merged
@lyrixx
Copy link
Member
lyrixx commented Apr 7, 2020

Hello. I open 2 PRs in order to fix the following statement:

That does not work in my case since 0, which is a valid value in my case, evaluates to false.

Using numeric for place name seems a bit odd to me. May I ask you why you prefer numeric over string with meanings?

The InMemoryMetadataStore class is also declared as final. Why?

What is your issue with this class?

@xabbuh xabbuh added the Workflow label Apr 7, 2020
lyrixx added a commit that referenced this issue Apr 7, 2020
@lyrixx
bug #36375 [Workflow] Use a strict comparison when retrieving raw mar…
932a4f8 
…king in MarkingStore (lyrixx)

This PR was merged into the 3.4 branch.

Discussion
----------

[Workflow] Use a strict comparison when retrieving raw marking in MarkingStore

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #36358
| License       | MIT
| Doc PR        |

Commits
-------

aebe8ae [Workflow] Use a strict comparison when retrieving raw markin in MarkingStore
lyrixx added a commit that referenced this issue Apr 7, 2020
@lyrixx
bug #36376 [Workflow] Use a strict comparison when retrieving raw mar…
e129a16 
…king in MarkingStore (lyrixx)

This PR was merged into the 4.4 branch.

Discussion
----------

[Workflow] Use a strict comparison when retrieving raw marking in MarkingStore

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix ##36358
| License       | MIT
| Doc PR        |

Commits
-------

a00a2f1 [Workflow] Use a strict comparison when retrieving raw marking in MarkingStore
@ghost
Copy link
Author
ghost commented Apr 7, 2020

Hello. I open 2 PRs in order to fix the following statement:

That does not work in my case since 0, which is a valid value in my case, evaluates to false.

Using numeric for place name seems a bit odd to me. May I ask you why you prefer numeric over string with meanings?

I am converting a legacy system, where states are numeric constants. For the time being, converting the state type is not possible. While not the best choice IMO, numeric constants for status are not unusual.

The InMemoryMetadataStore class is also declared as final. Why?

What is your issue with this class?

It is also final. It does not seem sensible to me, as it is not in any way an internal class. I have to copy'n'paste the whole class, if I want to modify its behavior.

@ro0NL
Copy link
Contributor
ro0NL commented Apr 7, 2020 •
edited
Loading

how would you hook-in using extending? the answer should be applicable using decoration as well :) (composition over inheritance).

@ghost
Copy link
Author
ghost commented Apr 7, 2020

Well you obviously follow some doctrine with final classes. I drop my case since I am neither a maintainer nor a contributor of this package.

@ghost ghost closed this as completed Apr 7, 2020
@VaN-dev
Copy link
VaN-dev commented Dec 7, 2020 •
edited
Loading

@lyrixx GuardEvent is also defined as final, which prevent from mocking it and unit-test the guard listeners receiving the event as argument. How are we supposed to unit test our guard listeners ? maybe you could provide a GuardEventInterface setting contract for isBlocked, setBlocked, getTransitionBlockerList and addTransitionBlocker methods ? so we could just mock the interface instead of the impl.

If this proposal is fine for you, i'd be happy to make a PR with that interface.

@lyrixx
Copy link
Member
lyrixx commented Dec 7, 2020

Hello,
Sure, you can unit test all listeners.
To do so, you don't need a mock, use the real object directly. It's much simpler :)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review Workflow
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0