What about supporting both ways (we would still need to think about good method names). That would not force the user to deconstruct already existing Transition instances.

By the way, you also need to update the constructor's docblock.

I'm not a fan of supporting both, but why not? I tend to prefer the BC break though, the commit has only three days and this is a lot nicer to me. I get used to not have any new in my code as much as possible :D.

I was more thinking about use cases where you are reusing parts on an existing workflow definition.

I understood #20451 was giving two possibilities. Either use one builder per definition, or keep using the same builder to add some transitions making the first a subset of the second. So I don't see the need to support what you're thinking about.

If the definition wasn't build through the builder there is nothing you could based the next definition on (except we made it possible to initialise the builder with an existing definition).

Ok got you, I'm going to fix this.

Status: needs work.

Status: needs review

Can you make fabbot happy too? :)

Ok comments addressed here, thanks!

Cant we introduce a TransitionBuilder? Imo. we should avoid union types (see also #20167 (comment)).

And I think exactly the same as you ;)

Perhaps should be avoided, but in this case (to me) it makes most sense from a configuring perspective.

But isn't this overkill just to avoid writing new Transition('foo', 'bar', 'baz)?

@HeahDude :)) but that affected direct user-land php configuration files, which made it considerable. And eventually the union type was avoided anyway (hence i think here it should also).

In this case we could convert arrays to transition objects in the FrameworkExtension (using its own builder).

Otherwise, maybe adding createTransition would be most simple..

$builder->addTransition($builder->createTransition($array));

Sorry but I fail to understand why...

$builder->addTransition($builder->createTransition($array))

// or
$builder->addTransition($builder->createTransition('foo', 'bar', 'baz'))

... is better/simpler/more convenient than:

 $builder->addTransition(new Transition(foo', 'bar', 'baz'));

It's not. But it could be useful to have it when transforming configs into objects.

You mean this line in FrameworkExtension would be:

 $transitions[] = array($transitionName, $from, $to);

Is that the only win for this PR?

Agree. I was already looking at it... this isnt actually needed to improve that.

So, we have a definition builder for user-land, what about a transition builder as well?

$t = (new TransitionBuilder())
  ->addTos($tos)
  ->addTo($to)
  ->build();

I've taking your comments into account already and started to work on it, I'm pushing it soon.

Again, why is

$t = (new TransitionBuilder())
  ->addTos($tos)
  ->addTo($to)
  ->build();

better/simpler than just:

$t = new Transition(foo', 'bar', 'baz');

Again.. it's not :) guess it's matter of preference? Imo. it would be just as useful for having the DefinitionBuilder?

Maybe we should re-consider that one first 😕 Ie. if we default the initial place to first available place directly in the Definition constructor, im not sure we really need a DefinitionBuilder as well.

As it was already removed from the framework extension for the same reason; overkill. Basically saying; it's practically not needed.

Reading the comments, I agree that we should keep things simple. I still don't understand which problems this PR is trying to solve... but I do understand it adds complexity to the code. 👎

@Nyholm that's not what I've in mind, tests are failing, but I've pushed my concept.

@fabpot I've just totally refactored it, one file to hold a new responsibility and a few lines changed.

I still fail to understand the issue and why this is a better than what we have today. Can you explain?

When using a DefinitionBuilder the code gets full of new Transition() instances, that feels wrong to me, but it's not really an issue.

But by introducing this transitions collection factory some tests are now failing because we use some definitions with invalid transitions and this actually looks like an issue IMHO.

So, you're saying that it' better to use an abstraction and hide the Transition class, which is a configuration instance. In any case, behind the scene, there is anyway a bunch of Transition instances that should be created. So, I prefer to be explicit an use Transition directly. Transition is a data object, it is its goal to help configuration. Still very much 👎 in this change.

Ok then, closing here. Thanks everyone for the feedback.

I am still not completely against this change, but I would rather provide two different methods. One were you pass a Transition instance and when that accept the plain values. This way we would make it possible to add transitions in both ways without the added complexity in the builder class. But we would still have to come up with some meaningful names to distinguish which method is responsible for what.

The more I think to this PR and to the DefinitionBuilder, the more I think the DefinitionBuilder is not really needed. It does not really help to reduce the number of LOC.

Example:

$transitions = [
    new Transition('name', 'A', 'B'),
    new Transition('name2', 'B', 'C'),
];

$definition = new Definition(['A', 'B', 'C'], $transitions);


(new DefinitionBuilder())
    ->addPlaces(['A', 'B', 'C'])
    ->addTransition(new Transition('name', 'A', 'B'))
    ->addTransition(new Transition('name2', 'B', 'C'))
;

What do you think?

Note: I'm agree with @fabpot about this PR and I'm also 👎 with its current state

@lyrixx It's not about the LOC, it's about being able to add transitions without passing them to the constructor and keeping the Definition immutable, so we should keep this builder IMHO.