What kind of extensions do you need for this object?

Any domain specific values associated to states/transitions. Descriptions, labels, email addresses associated to transitions, trigger type (system, user, admin), roles etc. See referenced ticket.

Note that adding any custom methods of properties in a child of one of our classes is forbidden by our BC promises anyway. So even if we remove the final keyword, adding new properties and methods is not something you should do.

It's not forbidden by your BC promises, you just don't guarantee BC in such cases, which is allright, since it's impossible

Meaning you still should not do it, and so I don't consider it as a valid use case for opening the class to inheritance.

Look, what's the alternative? I know you don't want inheritance because it's fragile, but that is precisely advantage for you in this case. Let me explain:

Workflow component is still new and I don't believe its API is stabilized. Workflow class currently depends on instance of Definition class

• If you change it so it starts to depend on Interface (which Definition implements) instead, that would mean that you won't be able to introduce any new (public) methods because of your BC promise.
• If you don't change it and continue to depend on concrete class, you can introduce new methods into Definition class without breaking your BC promise.

Either allow me to inherit the class, or you won't be allowed to extend it (introduce new public methods) in future.

I don't see how removing the final keyword (i.e. letting you inheriting the class) is necessary to be able to add new methods in the future in Symfony. The current codebase is the one where we can add whatever we want in Definition in Symfony without BC break (for instance implementing #23257).

I was comparing inheritance vs composition problem. If you don't allow users to do any changes then of course that's safest for you, but it's also middle finger for users of this component and I will rather stop using it in such case. There is plenty domain specific use cases which would never be accepted to core, because - they are domain specific!

The current codebase is the one where we can add whatever we want in Definition in Symfony without BC break

I don't see how removing final keyword prevents you doing the same thing.

Your only argument is that I should not subclass for my own safety but did not offer any alternative. I will gladly take the ridiculously low chance of introducing conflicting method name in my child though, so thank you for your concerns. I just want the ability to extend this class with domain specific features, so please explain your reluctance to this change and suggest better alternative. Thanks.

Well, if you have a need to attach additional info, you should rather put a comment on #23257 describing your use case, to give more input to the thinking about the proper way to solve the issue.

My main concern is that you want to advocate that the solution to your issue is using Symfony in an unsupported way (i.e. extending classes in ways not covered by our policy). This is not something solving the use case.
#23257 is precisely where the discussion happens to find a better alternative.

Only thing #23257 covers is to attach custom array with scalar values. That's good enough for general purpose, but it's not enough for my needs. I also want getter methods with type safety features for retrieving such data and validation of such data. That's not possible to specify via configuration.

I'm not advocating that this PR is solution for #23257 (notice empty fixed tickets column). That issue still needs to be done and I'm inclined to do it myself. But first stop trying to convince me that you know better than me what I need in domain of my application and let's merge this PR.

ping @lyrixx @Nyholm

Thank you for the ping.

I do understand you use case @ostrolucky. Many people has asked this before. We have answered that the Workflow is no place for arbitrary data. But I do understand it is super convenient to get your arbitrary data being sent all around the event system.

The supported way to do this is to store your arbitrary data some place else (any place) and then fetch it in the event listener. It may seams weird to split data that really belongs together (transition names and description) into two places... And yes, it is weird. It's correct, but weird.

Forgive me @stof, but "adding any custom methods of properties in a child of one of our classes is forbidden by our BC promises" means that we say: "Do it at your own risk". It is a risk no bundle SHOULD take, but @ostrolucky is obviously will to do so in his app.

I suggest we accept this PR. Let @ostrolucky and others be experimental, implement things in their app and find out what's good, what's working and make suggestions what is needed to be brought back into the component. Maybe they find that a transition should have a getDescription() or getEmail() function because it is important somehow. Give them the possibility be crazy, experimental and try new things out so they can make better arguments when they suggest features for this component in the future.

The "risk" we (Symfony/the Workflow component) take is minimal. We may get a complaint from some user that did not read our BC promise.

Wouldn't #23257 be a better approach? You don't mess with this "final" thing, but allow storing arbitrary data, so users can implement any of their needs. @lyrixx seems to be slightly in favor of that too: #23257 (comment)

Wouldn't #23257 be a better approach?

👍

Okey. I stand corrected then. =)

I've already explained why is #23257 not a solution here #23310 (comment). In summary, that ticket is for attaching only simple array with scalar values, not for attaching any custom methods.

I also want getter methods with type safety features for retrieving such data and validation of such data.

You can fetch that scalar values into your own value object with validation and own business logic. I don't see why you need to inherit the Definition class.

That's what I did. It creates fragile and messy API, duplications, complicates the whole thing and I need to introduce even more children (way to inject this into Workflow class, way to retrieve it, possibly override DefinitionBuilder).

Now, please don't comment here @unkind, you don't have any association to this component, unlike core symfony members and contributors to this component. There is already too much comments here. I won't continue discussing with you.

That's what I did.

How you did it if Definition class doesn't provide way yet to pass arbitrary data (#23257)?

Now, please don't comment here @unkind, you don't have any association to this component
I won't continue discussing with you.

Symfony community is not closed one, so I feel that it's OK if I share my thoughts regarding changes in the project which I actively use. However, it's your right to ignore me, I mostly share my thoughts for other people, so they can take into accounts different opinions.

@ostrolucky Even if @unkind is not part of the "Core Team", we do appreciate comments from the community. People like @unkind help a lot in reviewing/triaging and moving forward issues and pull requests. Without people like him, we would not be able to cope with Symfony's crazy activity.

I think you took it wrong way. I do appreciate comments from community and am aware of your stance regarding this. I am myself such a person. I just don't appreciate it in this specific case, because there is too much comments here and people started to talk about things I already answered and am forced to repeat myself, which just produces more comments and more readers who wouldn't read them all. Also I'm very well aware of @unkind's hard stance regarding final classes and if I was supposed to discuss about it with him, discussion would never end.

So what needs to be done here to come to resolution? I have answered all objections.

Hi @ostrolucky

How do you use this component? In a stand alone way or with the full-stack framework?
Because even if we merge this PR, in the full-stack framework, people who use the full-stack framework will not be able to leverage this feature (since everything is plugged together via the Bundle / DIC system (except if they use a compiler pass)).

Then could you describe us what you will add to the Definition class ?

Some users already asked to be able to give extra information to the workflow. But the targeted class were Transition and Place. Here you want to add extra information to the Definition class. And Right now I fail to see how you will use it since the definition is not given to the *Event class.

So could you describe us how you will use this "extended" Definition class ?

Thanks.

Hi @lyrixx

I use full stack framework. Change in this PR can be leveraged via Definition::setClass in compiler pass.

describe us what you will add to the Definition class

Not sure why you need to know that, I don't want you guys to focus on concrete problems. My core problem is that whatever it is that I need to add to Definition, I can't. Problems it focuses to solve are dynamic and will be changed with time. But anyway currently it is:

1. There is no class Place. Therefore next best place to put this information regarding places is Definition class
2. Definitoin class will also carry workflow information not related to states/transitions, such as description of workflow
3. I will also extend it with some methods we use internally, (for now it's just getFinalPlaces, which I might contribute upstream; it would be still good to be able to implement this without waiting for 6 months though)

definition is not given to the *Event class.

I can just inject workflow or workflow registry and pull definition from there. That said, currently I don't need this in event listeners.

Not sure why you need to know that,

I like to know what people do with the composant in order to improve it and also to take decisions on issues / PRs. I can not merge every single PR on the workflow because one person need it. I hope you understand why I'm acting like that.

So for now I have the same opinion on this PR as this comment in #23257.

But If we implement what you asked in #23257 this PR will not be needed except for the 3/. And I guess we will implement it...

can not merge every single PR on the workflow because one person need it.

Exactly. So allow for more generic way of extension (such as this PR) so user can build his own solution, according to his own needs in userspace, instead of forcing him to submit new feature requests each time (where some of them will be unavoidably denied) and waiting 6 months to have it in stable release. Community could even create bundle for this.

this PR will not be needed

Right, as I thought you are looking for reasons to deny this based on my very present needs and ignored my warning that these kind of things are dynamic in nature. Please don't be shortsighted. How probable do you think it is that there doesn't exist proper domain specific use case for extending this class?

Okay, currently #23257 will be enough for me (even though I doubt approved version will be the one I asked for). So you will close this PR right? And what will you do when after few months I or someone else will come back with another feature request which you won't accept, because this time it's too domain specific?

Not even sure why am I put in defensive position here for the whole time. It's like inquisition. You guys want from me all kind of explanations, but nobody could explain what's so bad about this change (argument about unsupport is ridiculous, 98.3% classes in Symfony are non-final and nobody is even asking you to officialy support this - quite contrary, I've expressed that I want to be able to do this at my own risk), or even why was this made final in the first place.

Except \Symfony\Component\Lock\Key, there is no other such case in Symfony (final class without any interface or parent class, which is required somewhere else via its typehint) so I expect you to have damn good reason for having it here. And for insisting to leave it as such, even after I demonstrated that there are reasons to extend it (in generic sense, not OOP), even after author of this final thing agreed with its removal.

If you are so uneasy about this, mark it @internal, mark all methods final, introduce interface, whatever, just give ability to extend/replace implementation.

How probable do you think it is that there doesn't exist proper domain specific use case for extending this class?

I assume that we don't understand why it should be done via inheritance:

/** @var $definition MyDefinition */ // Assumption that we have MyDefinition object
$definition = $workflow->getDefition();
$definition->customMethod();

// vs

$foo = Foo::fromArray($workflow->getDefiniton()->getData()); // #23257 
$foo->customMethod();
// or
$foo = new Foo($definition);
$foo->customMethod();
// or
$foo = new Foo($definition->getData('foo'), $definition->getData('bar'));
$foo->customMethod();

Isn't it the very same behavior, but decoupled from Symfony details? final does not actually limit you from doing the very same things, does it?

I've already responded to you here, do you really need to force same opinion of yours 3x?

You seem to be oblivious why I don't want to do this. I don't have time explaining everything wrong with this though, so hopefully just the most obvious thing will suffice: This "extension" approach forces consumer of application API to wrap the returned object into separate call every time it receives such object. It's totally inferior in comparison with other approaches.

Now let's hear the need for this class to be final. As I explained, it's very unique in Symfony codebase.

I've already responded to you here

Sure, but then I asked you how you did it if Definition class doesn't provide way yet to pass arbitrary data (#23257)? It makes me curious. Obviously, my example implies that #23257 is done.

"extension" approach forces consumer of application API to wrap the returned object into separate call every time it receives such object

Can you provide real-life example when it becomes a problem? I mean, creating objects is not that rare operation in OOP code. It happens quite often I'd say.

OK, let's assume that you need to inherit the Definition class with new methods. Can you provide some examples of these methods? It seems like several people want to know how your case looks like. What if these methods could be directly implemented in your domain w/o need in Definition class at all? It makes a lot of sense to describe the real example to make sure that the case is valid.

I mean, I saw the example like $definition->getEmail() above, but I don't see why Definition should contain an email. Definition becomes a value holder of all the things? This is a central detail of your architecture?

Moreover, I was in doubt since release of the Workflow component: how it helps to solve domain problems in comparison with DDD, for example? Your examples could be priceless contribution to the understanding of the component. Personally, I'd appreciate to see them, I'm open to new approaches.

Responding just because I don't see this moving. I don't like this discussion at all. I expect answer why does this change warrant 22+ comments when this class has no reason to be final in the first place.

Can you provide real-life example when it becomes a problem? I mean, creating objects is not that rare operation in OOP code. It happens quite often I'd say.

I don't think you understand. By consumer, I meant programmer. He is forced to transform that object by instantiating different one and supplying old one to it, by hand. Performance is not important here. It's painful violation of DRY. You always receive object you don't want, so you always have to transform it into different object, where you copy functionality of old one and add your own. Instead of always receiving object you want (old one, extended with functionality).

Can you provide
several people want to know

I did answer all of those. You would notice if you didn't spam this thread.

Rest of your answer is pure off topic. This isn't place to discuss usefulness of component or to satisfy your curiosity how did I implement something. Feel free to ask me that on slack.

@ostrolucky I'm not going to dive into the actual change you request because I don't know that part of the code. Yet if you allow me to try helping you, your tone is really not encouraging anyone to pursue the discussion. It is very common for reviewers to ask for example use cases and alternatives, so they can get the pov of the submitter and help make the best move. Here it's like you're saying loudly "trust me you idiots". I'm not sure that's the best plan for getting anything merged... The arguments you posted have to be carefully understood. You may have understood faster than anyone else why this change is best. But saying "nothing is final anywhere" (which is not accurate, see eg CacheItem) is not really an argument; and refusing to dive into the use cases and discuss about the alternatives because it's "off topic" neither (it's not.)

So, I'm kindly asking any reviewers to ignore your past tone and focus on the actual technical arguments. And kindly asking you to be more tactical in your communication style if you want people to care about this PR at all (that's everyone's free time, so better be friendly to each others...)

Just my 2cts :)

Thank you for you wise words. I didn't want to come off as arrogant. Try to understand my frustration though. For a while, I did my best to respond to everybody asking for more information related to this change. Yet, people don't notice that and ask me same thing over and over. I have answered queries regarding use cases in these comments: #23310 (comment), #23310 (comment), #23310 (comment), #23310 (comment). Meanwhile, nobody answered my queries. It's exhausting to defend myself over and over without seeing a reason. Care to help dispute at least some of these conclusions of mine?

• Noise ratio in this thread is too high
• This is a low risk change
• This change is mutually beneficial for both users and maintainers:
  • Users: can immediately extend functionality of this part of workflow component, don't need to wait for upstream to implement it, or create fork. Can even provide bundle for adding functionality.
  • Maintainers: don't have to deal with increased amount of feature requests and codebase resulting from it, can instead give generic advice how to extend functionality on user's own
• Nobody knows reason why is this class final
• Author of changeset which added final keyword in this class agreed with its removal
• This class being final is inconsistent with the way all the rest of the Symfony code is written. Once again, I'm not saying nothing is final anywhere. What I'm saying is that dependency for final class is very uncommon (only 1 other such case), therefore should have strong reason to exist.
• Current implementation breaks open/closed principle and dependency inversion principle
• I have provided some valid use cases
• Some of the use cases are not going to be implemented into core
• In case of use case not accepted into core, user is left with sub par options to solve it
• In case of use case accepted into core, user needs to wait XY months until next minor version is released, or jeopardize stability of the project by using unstable version
• [Workflow] Allow to define arbitrary data for states/transitions #23257 doesn't look like is going to be accepted in proposed way due to BC
• article When to declare classes final recommends pretty defensive approach by default, but even this article says the way it was used here is wrong. So I am not alone.

This "extension" approach forces consumer of application API to wrap the returned object into separate call every time it receives such object.
...
He is forced to transform that object by instantiating different one and supplying old one to it, by hand.

One more attempt to understand the use case: is it possible to create DI-factory which gets the Definition object and returns object of your class, e.g.

return function (ContainerConfigurator $c) {
    $s->set(Foo::class)
        ->factory(function (Definition $definition) {
            return new Foo(
                $definition,
                $definition->getData('bar'),
                $definition->getData('baz')
            );
        })
        ->argument(ref('definition')); // or DefinitionBuilder, for example, not sure
};

(I'm trying to mimic proposed format for DI configuration: #23834 (comment))
?

Doesn't it fix your issue with "transformation by hand" "every time"? Since Definition is immutable object, it seems like a appropriate solution, isn't it?

Current implementation breaks open/closed principle and dependency inversion principle.

Care to explain this one? The open/closed principle states that an object should be open to extension (in any form, including composition or decorating) but closed for modification.

The only way this would break that principle is when this object is to open, relies to much on instable functionality. Or doesn't allow to change the functionality of something using eg. DI.

The "dependency inversion principle" doesn't apply here as this object has no direct dependencies.
If it would rely on external classes for the functioning of this class, and this functionality is hardcoded (without a proper reason to be so) then this would break the principle.

I consider making a class final a very good thing actually. It makes the code easier to change (by maintainers) (and be modernized) and prevents developers from making "bad" decisions. Having a class be "open" allows the developer do to whatever they want, which is not exactly what the designer of the class/system had in mind, and can complicate changes in the future.

And the "I will take that risk" is very short sighted, this class isn't just open to you, but everyone else who assumes it's safe to extend this and add any functionality they want. Then the maintainer improves the class, and people start to explain the maintainer just broke there code... (which is why Symfony has a BC promise in place).

Removing the final on a class without a good proper, well thought out reason, feels more like a "there I fixed your problem" solution. The proposed solutions work just as good (as you indicated), you can add any form of additional information (including a values container of your own to store structured strict-typed data).

It's not so much there is no solution, it's more you're not happy with "the" solution, but they can in fact work. Or as Paul M. Jones states There are no solutions, only tradeoffs.

Systems like these work best with a generic metadata-attribute where you can store additional information and rules. And allows for enough flexibility, without having to open the class for everyone and the risk that introduces.

I have provided some valid use cases.

Which can be perfectly solved using the proposed solutions (in a another issue), secondly. You can also add your metadata system and invoke this when transitioning, it will require a bit more work to implement but does solve your problem.

article When to declare classes final recommends pretty defensive approach by default, but even this article says the way it was used here is wrong. So I am not alone.

To quote from the article:

2. There will be less stuffing functionality in existing code via inheritance, which, in my opinion, is a symptom of haste combined with feature creep.

3. Force the developer to think about user public API
   Developers tend to use inheritance to add accessors and additional API to existing classes:

5. A final class can always be made extensible

Coding a new class as final also means that you can make it extensible at any point in time (if really required).

No drawbacks, but you will have to explain your reasoning for such change to yourself and other
members in your team, and that discussion may lead to better solutions before anything gets merged.

So you may want to explain this in a bit more detail. The Definition can be considered a value-object (though still mutable).

Of course there ARE solutions to solve my problem. For instance, I can just override Workflow#getDefinition(). I can replace class via composer configuration. I can create fork of this component. I can replace it with another library. Or I can write factory as you suggested (and yeah, call it every goddamn time I need my custom object). Each of this solution just circumvents the issue. I wanted to solve root of the problem. But I see everybody think it's ok in symfony components to have dependencies on final classes with no reason. I feel 0 support for this change, so I give up.