[Workflow] CS tweaks #19187
[Workflow] CS tweaks #19187
Conversation
| @@ -59,10 +59,6 @@ public function getMarking($subject) | |||
| { | |||
| $marking = $this->markingStore->getMarking($subject); | |||
|
|
|||
| if (!$marking instanceof Marking) { | |||
There was a problem hiding this comment.
This should stop imo :-)
There was a problem hiding this comment.
Why did you removed this code ?
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm 👎 with this change. Nothing in PHP 5 forbid someone to violate the return type of an interface.
There was a problem hiding this comment.
I guess we're not gonna agree on this. However i understand the concerns when it comes to "missing the contract and debugging", but the exception would be cosmetic only. Ie. the plain error you get 9/10 is just as informative (stacktrace wise), eventually you'll have to debug your implementation anyway. And if error handling is poor, i would advice fixing that.
is it common for users to write their own implementation of the interface
From a symfony/framework pov this is almost any interface... which is exactly why i think we have to draw the line on this.
For debugging/validating on return types; i think this should be be mostly covered by static code analysis, tests, and eventually PHP7.
This is why i changed it. :) Long story short: (imo) it's not symfony's responsibility but yes, it's DX related.
There was a problem hiding this comment.
I agree with @nicolas-grekas here. We don't have that many interfaces in Symfony that should almost always be implemented by the end user.
There was a problem hiding this comment.
I think this interface is no different then most others. Its a pillar interface in the component with prepackaged/generic implemenations which will gonna be used 9/10.
DX-wise, the real wtf happens when PropertyAccess fails from these marking stores. You'll get an exception thats hard to relate to what you're probably doing. I also see no real added value to try/catch those and throw a WorkflowException from a previous exception, but that at least would be a true DX improvement imho.
Like i said, we're not gonna agree on this. So if symfony team votes >50% i'll remove/keep it then. Or however it's decided normally :)
There was a problem hiding this comment.
@ro0NL there is no downside into keeping this, please revert this change, let's move on. We won't have a vote on this but on the PR as a whole, as always.
There was a problem hiding this comment.
OK. Im still not convinced though. Anyway hope to get some more feedback/thumbs this day then, and i'll have a look tonight. However i feel my arguments are not taken seriously.
Dont get me wrong;
on the PR as a whole
From this debate on there's no clear yes/no till now, if you ask me, besides two 👍 for the the PR. Anyway, to move on, ill revert it from a "out of scope of this PR" point of view. Under protest.
| @@ -68,7 +66,7 @@ public function addPlace($place) | |||
| throw new InvalidArgumentException(sprintf('The place "%s" contains invalid characters.', $place)); | |||
| } | |||
|
|
|||
| if (!count($this->places)) { | |||
There was a problem hiding this comment.
This is up for debate and maybe out of scope here.. but seems in line with https://github.com/symfony/symfony/pull/19187/files#diff-6d1e8dc0d2c0e7c4ab206d0efbb71516R63 😅
|
I'm again
8000
st adding |
|
@fabpot do you find the custom SPL exceptions useful anyway? Btw;
|
|
@ro0NL custom exceptions made easier global catching. It's done like that in every other components. |
|
Thats not the point ;-) A exception pointer interface per component is just fine, but do we need custom 1on1 SPL exceptions all the time, ie. an I see no added value in globally catching logic errors (ie programmers faults), however currently logic is thrown where its not necessarily a programmers fault. Anyway, totally out-of-scope, and guess also not something were gonna agree on :-) |
|||
|
Reverted the the Anyway, if this decision was rational, i guess its good to go 👍 |
| @@ -59,6 +59,10 @@ public function getMarking($subject) | |||
| { | |||
| $marking = $this->markingStore->getMarking($subject); | |||
|
|
|||
| if (!$marking instanceof Marking) { | |||
| throw new LogicException(sprintf('The value returned by the MarkingStore is not an instance of "%s" for workflow "%s".', Marking::class, $this->name)); | |||
There was a problem hiding this comment.
For being a contract check, i guess LogicException is just fine also :) (opposed to UnexpectedValue) 😕
|
👍 |
1 similar comment
|
👍 |
|
👍 :) |
|
Thank you @ro0NL. |
Just a few minor tweaks/fixes after playing around with it. Some phpdoc should still be added imo, like missing
@throws, right?