Follow up to #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface.

CommentFileSizeAuthor
#16 workflows-2897134-16-enforce_numeric_weights_workflow_state.patch3.24 KBMaskOta
#16 interdiff.txt1.94 KBMaskOta
#15 interdiff.txt2.71 KBsam152
#14 workflows-2897134-enforce_numeric_weights_workflow_states.diff3.24 KBMaskOta
#11 interdiff.txt1.27 KBdinesh18
#11 2897134-11.patch3.1 KBdinesh18
#5 2897134-5.patch3.05 KBsam152

Comments

Sam152 created an issue. See original summary.

tacituseu’s picture

tacituseu commented
Title: Assert weights are numeric when settings state/transition weights. » [PP-1] Assert weights are numeric when settings state/transition weights.
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: [PP-1] Assert weights are numeric when settings state/transition weights. » Assert weights are numeric when settings state/transition weights.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sam152’s picture

sam152 commented
Status: Active » Needs review
StatusFileSize
new 2897134-5.patch3.05 KB

Noticed a stray quote in some related exceptions.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

The patch does exactly what's being asked for in the IS. Looks good to me.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Enforce that weights are numeric when settings state/transition weights. » Enforce that weights are numeric when settings state/transition weights 
+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -178,7 +178,10 @@ public function setStateLabel($state_id, $label) {
+      throw new \InvalidArgumentException("The weight '$weight' is not numeric.");

@@ -390,7 +393,10 @@ public function setTransitionLabel($transition_id, $label) {
+      throw new \InvalidArgumentException("The weight '$weight' should be numeric.");

Is there any particular reason we're using different messages for the state weight vs. transition weight?

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

FWIW I think both could be:

The weight '$weight' must be numeric.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

Actually, ideally, the exception would also tell you which state or transition has a non-numeric weight, for better debugging.

So something like:

The weight '$weight' must be numeric for state '$label'.

Setting NW for that. Thanks!

dinesh18’s picture

dinesh18 commented
Status: Needs work » Needs review
StatusFileSize
new 2897134-11.patch3.1 KB
new interdiff.txt1.27 KB

Here is an updated patch and interdiff implemented as per #10 comment.

sam152’s picture

sam152 commented
Status: Needs review » Needs work

Hi @Dinesh18, looks good, but I think we'll have to update the tests to assert the updated exceptions.

sam152’s picture

sam152 commented
Issue tags: +Novice
MaskOta’s picture

MaskOta commented
Status: Needs work » Needs review
StatusFileSize
new workflows-2897134-enforce_numeric_weights_workflow_states.diff3.24 KB

My first test fixes. Please be gentle test bot.

sam152’s picture

sam152 commented
Status: Needs review » Needs work
StatusFileSize
new interdiff.txt2.71 KB

Hi @MaskOta, thanks for the contribution! Sometimes it's helpful to provide an interdiff, which are the changes you've introduced since the last patch. I've included one for this issue for between #11 and #14. You can generate these with git while you're working.

+    if (!is_numeric($weight)) {
+      $label = $this->getState($state_id)->label();
+      throw new \InvalidArgumentException("The weight '$weight' must be numeric for state '$label'.");
     }
...
+    if (!is_numeric($weight)) {
+      $label = $this->getTransition($transition_id)->label();
+      throw new \InvalidArgumentException("The weight '$weight' must be numeric for state '$label'.");
     }

Both these calls to getState and getTransition are guarded by a hasState/hasTransition check, so totally safe to use them for the purposes of getting the label. +1

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -390,7 +394,11 @@ public function setTransitionLabel($transition_id, $label) {
+    if (!is_numeric($weight)) {
+      $label = $this->getTransition($transition_id)->label();
+      throw new \InvalidArgumentException("The weight '$weight' must be numeric for state '$label'.");
     }

One last thing here, we're talking about transitions in this method and the exception references 'state'.

MaskOta’s picture

MaskOta commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.94 KB
new workflows-2897134-16-enforce_numeric_weights_workflow_state.patch3.24 KB

Thanks for reviewing. I fixed up the labels for the transition so they make more sense.

sam152’s picture

sam152 commented
Status: Needs review » Reviewed & tested by the community

Perfect, thanks!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +DrupalSouth 2017

Committed as 7dbf76d and pushed to 8.5.x

  • larowlan committed 7dbf76d on 8.5.x 
    Issue #2897134 by MaskOta, Sam152, Dinesh18, xjm: Enforce that weights...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.