Problem/Motivation

Follow-up to #2899553: Architectural review of the Workflows module (documentation cleanups).

In \Drupal\workflows\Form\WorkflowAddForm::form

 $workflow_types = array_map(function ($plugin_definition) {
      return $plugin_definition['label'];
    }, $this->workflowTypePluginManager->getDefinitions());

could just use array_column.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 2902018-18.patch752 bytesvijaycs85
#14 interdiff-4-11.txt2.59 KBharsha012
#11 2902018-11.patch2.97 KBharsha012
#9 2902018-09.patch751 bytesharsha012
#4 arraycolumn_2902018_4.patch2.94 KBmeenakshig
#3 arraycolumn_2902018_1.patch640 bytesmeenakshig

Comments

Sam152 created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Issue summary: View changes
Issue tags: +Workflow Initiative
meenakshig’s picture

meenakshig commented
Status: Active » Needs review
StatusFileSize
new arraycolumn_2902018_1.patch640 bytes

changed array_map to array_column. please let me know is it correct ?

meenakshig’s picture

meenakshig commented
StatusFileSize
new arraycolumn_2902018_4.patch2.94 KB

sorry my previous patch arraycolumn_2902018_1.patch was not correct i have changed array_map to array_column only in one place so here i have changed it where ever possible

amateescu’s picture

amateescu commented
Status: Needs review » Needs work

@Meenakshi Gupta, the array_column() function is not a direct replacement for array_map(), they do entirely different things :) I linked each of them to their documentation on php.net so you can see exactly how they should be used.

amateescu’s picture

amateescu commented
Title: Use array_column in place of array_map where possible. » Use array_column instead of array_map where possible in the Workflows module

Maybe the issue title was confusing...

meenakshig’s picture

meenakshig commented
Status: Needs work » Needs review

@amateescu thanks for clarifying :)

meenakshig’s picture

meenakshig commented
Status: Needs review » Needs work
harsha012’s picture

harsha012 commented
Status: Needs work » Needs review
StatusFileSize
new 2902018-09.patch751 bytes

Do we need to change all the array_map() to array_column() in the workflow module ?

i have fixed the array_column() syntax in WorkflowAddForm.php

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs review » Needs work

Not all array_map() functions make sense to move to array_column(), but if they do I say lets change them.

+++ b/core/modules/workflows/src/Form/WorkflowAddForm.php
@@ -65,9 +65,8 @@ public function form(array $form, FormStateInterface $form_state) {
+    $workflow_types = array_column(
+     $this->workflowTypePluginManager->getDefinitions(), 'label');

These should be on the same line.

harsha012’s picture

harsha012 commented
Status: Needs work » Needs review
StatusFileSize
new 2902018-11.patch2.97 KB

fixed as per #10

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

I don't think we can change the calls to \Drupal\workflows\State::labelCallback, this doesn't return "label", it returns the label of a state.

Please provide an interdiff with patches. https://www.drupal.org/documentation/git/interdiff

The last submitted patch, 9: 2902018-09.patch, failed testing. View results

harsha012’s picture

harsha012 commented
StatusFileSize
new interdiff-4-11.txt2.59 KB

interdiff

Status: Needs review » Needs work

The last submitted patch, 11: 2902018-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vijaycs85’s picture

vijaycs85
Location London, UK
commented

I don't think it makes sense to update any place other than addForm and current patch needs a change to make machinename/id as key:

diff --git a/core/modules/workflows/src/Form/WorkflowAddForm.php b/core/modules/workflows/src/Form/WorkflowAddForm.php
index 66227fe..1989469 100644
--- a/core/modules/workflows/src/Form/WorkflowAddForm.php
+++ b/core/modules/workflows/src/Form/WorkflowAddForm.php
@@ -64,7 +64,8 @@ public function form(array $form, FormStateInterface $form_state) {
       ],
     ];

-    $workflow_types = array_column($this->workflowTypePluginManager->getDefinitions(), 'label');
+    $workflow_types = array_column($this->workflowTypePluginManager->getDefinitions(), 'label', 'id');
+
     $form['workflow_type'] = [
       '#type' => 'select',
       '#title' => $this->t('Workflow type'),
sam152’s picture

sam152 commented 
+++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
@@ -199,7 +199,7 @@ public function form(array $form, FormStateInterface $form_state) {
-          '#items' => array_map([State::class, 'labelCallback'], $transition->from()),
+          '#items' => array_column($transition->from(), 'label'),

+++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
@@ -77,7 +77,7 @@ public function form(array $form, FormStateInterface $form_state) {
-    $states = array_map([State::class, 'labelCallback'], $workflow->getTypePlugin()->getStates());
+    $states = array_column($workflow->getTypePlugin()->getStates(), 'label');

+++ b/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
@@ -92,7 +92,7 @@ public function form(array $form, FormStateInterface $form_state) {
-    $states = array_map([State::class, 'labelCallback'], $workflow->getTypePlugin()->getStates());
+    $states = array_column($workflow->getTypePlugin()->getStates(), 'label');

Yup, none of these can use array_column because they are a method call on an object, not accessing an array value.

@vijaycs85 is also correct, the third param should be 'id' to be used as the array key.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Needs work » Needs review
StatusFileSize
new 2902018-18.patch752 bytes

Thanks for confirming @Sam152. Here is a patch.

sam152’s picture

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

Looks good to me.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Crediting @Sam152, @timmillwood and @amateescu for reviews and mentoring.

  • larowlan committed 4210adf on 8.5.x 
    Issue #2902018 by harsha012, Meenakshi Gupta, vijaycs85, Sam152,...

  • larowlan committed 73fd8e2 on 8.4.x 
    Issue #2902018 by harsha012, Meenakshi Gupta, vijaycs85, Sam152,...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed as 4210adf and pushed to 8.5.x.
Cherry-picked as 73fd8e2 and pushed to 8.4.x.

Status: Fixed » Closed (fixed)

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