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

CommentFileSizeAuthor
#5 address_performance-2897133-5.patch2.85 KBsam152
#5 interdiff.txt1.59 KBsam152
#3 address_performance-2897133-3.patch1.26 KBlarowlan

Comments

Sam152 created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » larowlan
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned
Status: Active » Needs review
StatusFileSize
new address_performance-2897133-3.patch1.26 KB

Sorry for hating on elseif, I think this is easier to understand the flow and is more performant because it deletes and moves on early

sam152’s picture

sam152 commented
Assigned: Unassigned » sam152 
+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -229,15 +229,20 @@ public function deleteState($state_id) {
+        if (empty($transition['from'])) {
+          // There are no more 'from' entries, remove the transition.
+          $this->deleteTransition($transition_id);
+          continue;
+        }

I wanted to make sure we had test coverage for this, so I stuck a debugger in here and it didn't hit once when running all of the unit tests. Working on a test for this.

sam152’s picture

sam152 commented
StatusFileSize
new interdiff.txt1.59 KB
new address_performance-2897133-5.patch2.85 KB 
  1. +++ b/core/modules/workflows/tests/src/Unit/WorkflowTest.php
@@ -210,14 +210,18 @@ public function testDeleteState() {
+    $this->assertCount(2, $workflow_type->getState('published')->getTransitions());

    The existing line that proves 'create_new_draft' was removed when the 'draft' state was removed. 

  2. +++ b/core/modules/workflows/tests/src/Unit/WorkflowTest.php
@@ -210,14 +210,18 @@ public function testDeleteState() {
+    $workflow_type->deleteState('published');
+    $this->assertCount(0, $workflow_type->getTransitions());

    The new lines that prove now that 'published' and 'draft' are gone, the 'archive' transition was deleted, because all 'from' states were removed.

The fix itself LGTM. +1 RTBC.

sam152’s picture

sam152 commented
Assigned: sam152 » Unassigned

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: Needs review » Reviewed & tested by the community

I only tweaked the test, maybe I can RTBC this.

  • catch committed 6297419 on 8.5.x 
    Issue #2897133 by Sam152, larowlan: Address performance issues in \...

catch credited catch.

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed b42ef6b on 8.4.x 
    Issue #2897133 by Sam152, larowlan: Address performance issues in \...

Status: Fixed » Closed (fixed)

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