Problem/Motivation

We mention 'to' and 'from' in the workflows interfaces, lets move these to constants.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 interdiff.txt1.16 KBeric115
#14 2896724-14.patch2.77 KBeric115
#12 interdiff.txt1.22 KBeric115
#12 2896724-12.patch2.7 KBeric115
#9 interdiff.txt1.99 KBeric115
#9 2896724-9.patch2.78 KBeric115
#6 2896724-6.patch1.22 KBkim.pepper

Comments

Sam152 created an issue. See original summary.

sam152’s picture

sam152 commented
Issue tags: +workflows.module
sam152’s picture

sam152 commented
Issue tags: +Workflow Initiative
sam152’s picture

sam152 commented
Component: content_moderation.module » workflows.module
Issue tags: -workflows.module

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.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
StatusFileSize
new 2896724-6.patch1.22 KB

I could only see one use of the string in the interfaces.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Minor
Status: Needs review » Needs work
Issue tags: +DX (Developer Experience)

We should also update 

   * @param string $direction
   *   (optional) The direction of the transition. Defaults to 'from'. Possible
   *   values are: 'from' and 'to'.
sam152’s picture

sam152 commented
Issue tags: +Novice
eric115’s picture

eric115 commented
StatusFileSize
new 2896724-9.patch2.78 KB
new interdiff.txt1.99 KB

Updated the comment and found one more occurrence of 'from'.

I used the full namespace in the comments to try and follow some other examples in core, although one line goes goes over 80 characters.

eric115’s picture

eric115 commented
Status: Needs work » Needs review
sam152’s picture

sam152 commented 
+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -248,13 +248,17 @@ public function getTransitions(array $transition_ids = NULL);
+   *   \Drupal\workflows\TransitionInterface\TransitionInterface::DIRECTION_FROM.
...
+   *   \Drupal\workflows\TransitionInterface\TransitionInterface::DIRECTION_FROM
...
+   *   \Drupal\workflows\TransitionInterface\TransitionInterface::DIRECTION_TO.

Maybe we could use just TransitionInterface::DIRECTION_FROM in the @param comments and put the whole class in a @see at the end of the comment.

Otherwise looks good to me.

eric115’s picture

eric115 commented
StatusFileSize
new 2896724-12.patch2.7 KB
new interdiff.txt1.22 KB

Updated the patch as per @Sam152's comments in #11.

sam152’s picture

sam152 commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -248,13 +248,17 @@ public function getTransitions(array $transition_ids = NULL);
-   *   (optional) The direction of the transition. Defaults to 'from'. Possible
-   *   values are: 'from' and 'to'.
+   *   (optional) The direction of the transition.
+   *   Defaults to TransitionInterface::DIRECTION_FROM.
+   *   Possible values are:
+   *   TransitionInterface::DIRECTION_FROM or TransitionInterface::DIRECTION_TO.

    Should probably read as one sentence without the premature line breaks. 

  2. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -248,13 +248,17 @@ public function getTransitions(array $transition_ids = NULL);
+   * @see \Drupal\workflows\TransitionInterface

    I think we can @see the full path \Drupal\workflows\TransitionInterface\TransitionInterface::DIRECTION_TO and add a line for each constant.

eric115’s picture

eric115 commented
StatusFileSize
new 2896724-14.patch2.77 KB
new interdiff.txt1.16 KB

New patch with feedback from #13.

eric115’s picture

eric115 commented
Status: Needs work » Needs review
sam152’s picture

sam152 commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

Looks good to me!

Since workflows will have been stable for 6 months before this is in a release, I think a change record might make sense here.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Now I think we just need to use the constants in these places

\Drupal\workflows\Plugin\WorkflowTypeBase::deleteState
\Drupal\workflows\Plugin\WorkflowTypeBase::addTransition
\Drupal\workflows\Plugin\WorkflowTypeBase::getTransition
\Drupal\workflows\Plugin\WorkflowTypeBase::getTransitionIdFromStateToState
\Drupal\workflows\Plugin\WorkflowTypeBase::setTransitionFromStates

@Sam152 - do you agree?

sam152’s picture

sam152 commented

I'm not totally sure, I think it makes sense to use these constants when calling a public method from the outside, but not as much when accessing some internal configuration structure in an implementation of the plugin. You could satisfy the plugin interface while not storing configuration in those keys, so I don't know if using the constant there indicates the schema is fixed in some way.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

@eric115 @kim.pepper @WimLeers - thoughts?

Happy to go with @Sam152 (maintainer) if others agree

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Reviewed & tested by the community

I'm happy to go with @Sam152

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Crediting Sam and Wim for their reviews.

We still need a change record as per request from Sam.

eric115’s picture

eric115 commented

Added a change record https://www.drupal.org/node/2914791

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Thanks @eric. Slight change: I think you meant to use TO instead of duplicating FROM:

Before:

getTransitionsForState('state_id', 'from');
getTransitionsForState('state_id', 'from');
After:

getTransitionsForState('state_id', TransitionInterface::DIRECTION_FROM)
getTransitionsForState('state_id', TransitionInterface::DIRECTION_FROM)
eric115’s picture

eric115 commented

Oops, missed that one!

All fixed :)

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

  • larowlan committed 4a80851 on 8.5.x 
    Issue #2896724 by Eric115, kim.pepper, Sam152, Wim Leers: Create...
larowlan’s picture

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

Committed as 4a80851 and pushed to 8.5.x.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

published change record

Status: Fixed » Closed (fixed)

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