I'm sorry, but I fail to see why adding this interface will allow us to introduce an ImmutableDefinition. And even how we could implement that.

More over, I played with your PR, and I did not have the need to make workflow.[name].definition service public.

I want to introduce the interface because all symfony classes only need the getters. Having a lot of functions that are never used is a violation for Interface segration principle. I believe this is the argument for introducing the interface.

A consequence of introducing the interface would be that we could allow the definition to be a service. The reason you want the definition as a service is because we want to use a Dumper. Currently there is no way to configure a definition in the config.yml and then dump it with Graphviz.

Currently there is no way to configure a definition in the config.yml and then dump it with Graphviz.

Yes, it's possible. You can get the service workflow.xxx and then use the reflection on it.

And IMHO, the definition should not be a service. it does nothing. It's just a data object that contain the configuration. It's OK for me to inline it inside a Workflow instance.

Fair. As long as it is possible.

So is this how we recommend people to dump the Definition?

$workflow = $this->get('workflow.xxx');
$reflWorkflow = new ReflectionClass(workflow);
$reflProp = $reflWorkflow->getProperty('definition');

$reflProp->setAccessable(true);
$definition = $reflBaz->getValue(workflow);

$output = (new GraphvizDumper())->dump($definition);

If they use the service, it means they are using the full stack framework, and so everything is already done in the command.

If they don't use the full stack framework, it means they do everything by hand. so they have access to $definition.

I see, I did not know of the Symfony command.

Then there is no issue. Thank you.

Having a lot of functions that are never used is a violation for Interface serration principle

The reasoning behind ISP is not just "having a lot of functions that are never used", but ability to implement them independently in the first place. It's a very good sign of violation when you work with services, but data objects (entities, value objects) usually do not have different implementations, it's kinda weird to have 2 implementations of the Definition class.

However, I agree that mutability seems like flaw in the original design, because Definition looks like a value object. But even if you want to see ImmutableDefinition, I don't get why do you need to have common interface for both classes (Definition, ImmutableDefinition), especially if you always can convert between them?

For me, it looks similar to the following issue: http://verraes.net/2016/02/type-safety-and-money/#parent-interface
You have 2 different concepts, you always know what do you need to type hint against.

However, I agree that mutability seems like flaw in the original design

Unfortunately, PHP object are mutable ...

Unfortunately, PHP object are mutable

If you don't add mutable methods, then it isn't.

Hehe, indeed. But adding such feature will make the code harder to read.

@lyrixx why so? I personally find easier to work with immutable objects as you don't have to watch for the side effects. There is obviously cases where you don't want immutability, but is there such a thing in the workflow component?

@theofidry feel free to open a PR.

is it ok to introduce BC breaks?

If there is a real design issue, yes. But you have to be very quick because we already reached the feature freeze. After the release of 3.2, it will be too late.

If it's worth it, I'll try to do it this week then :)

Sure, or add a DefinitionBuilder which builds an immutable definition.

I really prefer the builder.

I can give the builder a try.. Just a moment

From: Grégoire Pineau notifications@github.com notifications@github.com
Reply: symfony/symfony
reply@reply.github.com
reply@reply.github.com
Date: 8 november 2016 at 19:24:09
To: symfony/symfony symfony@noreply.github.com
symfony@noreply.github.com
Cc: Tobias Nyholm tobias.nyholm@gmail.com tobias.nyholm@gmail.com, State
change state_change@noreply.github.com state_change@noreply.github.com
Subject: Re: [symfony/symfony] [Workflow] Added DefinitionInterface
(#20441)

I really prefer the builder.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#20441 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABN1Rg3h4tNXMPCft6OCRK-MKfBwuD-yks5q8L5JgaJpZM4KroPF
.

I'd go with with-er syntax (PSR-7-like) as it makes value object more cohesive. And builder looks more complicated (what's the benefit?).

See #20451