Problem/Motivation

When displaying a list of entities, using views, which are rendered using a the field render entity with a selected view mode and all the rendered entities include a state transition form. Then all the form on the pages will only update the first rendered entity.

Proposed Resolution

Allow each form instance to have its own form ID.

Example

Here is an example of how this issue might occur, the screenshot below shows a transition form for each order item:

Order with order items and transition buttons

CommentFileSizeAuthor
#5 Screenshot from 2017-07-12 10-29-12.png55.85 KBjosephdpurcell
#4 interdiff.txt504 bytesjosephdpurcell
#4 state_machine-multiple-state-transition-form-same-page-form-id-conflict-2860279-4.patch1.08 KBjosephdpurcell
#2 state_machine-multiple-state-transition-form-same-page-form-id-conflict-2860279-2.patch1.5 KBnikathone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nikathone created an issue. See original summary.

nikathone’s picture

nikathone
Primary language Swahili
Location Ontario
CreditAttribution: nikathone at GiantGoat Web Development Inc. commented
Status: Active » Needs review
FileSize
state_machine-multiple-state-transition-form-same-page-form-id-conflict-2860279-2.patch1.5 KB

And here is the patch. Took the same approach as in https://github.com/drupalcommerce/commerce/blob/8.x-2.x/modules/cart/src... but this will need anybody implementing a hook_form_FORM_ID_alter() to update their code if accepted.

smaz’s picture

smaz
Primary language English
Location England, UK
CreditAttribution: smaz at Five Mile Media commented
Status: Needs review » Needs work

I had this issue on a view, and the patch provided fixes the issue - many thanks.

I've marked needs work, for the following (a nitpick really):

   public function getFormId() {
-    return 'state_machine_state_transition_form';
+    $form_id = 'state_machine_state_transition_' . self::$formInstanceId;
+    return $form_id . '_form';
   }

Can we not shorten that to just:

return 'state_machine_state_transition_' . self::$formInstanceId;

If not for whatever reason, then feel free to mark this as RTBC!

josephdpurcell’s picture

josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commented
Status: Needs work » Reviewed & tested by the community
FileSize
state_machine-multiple-state-transition-form-same-page-form-id-conflict-2860279-4.patch1.08 KB
interdiff.txt504 bytes

I ran into this issue as well. The patch from #2 works.

Putting the increment in the constructor should be avoided, but I am not aware of a better pattern for addressing this type of issue.

I'm uploading a patch that implements the suggestion in #3, and setting to RTBC.

josephdpurcell’s picture

josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commented
Issue summary: View changes
FileSize
Screenshot from 2017-07-12 10-29-12.png55.85 KB

Updated description with an example of how this problem might arise.

jcandan’s picture

jcandan CreditAttribution: jcandan as a volunteer commented

#4 is working great!

uniquename’s picture

uniquename CreditAttribution: uniquename commented

works fine for me!

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Version: 8.x-1.0-beta3 » 8.x-1.x-dev
Assigned: Unassigned » bojanz
Status: Reviewed & tested by the community » Needs work

We did the static counter approach in Commerce, and ended up with a bunch of bugs, where the static wasn't updated properly on #ajax.
We had to replace it with a more stable solution. So, let's use the entity type and ID here instead of the static counter.

  • bojanz committed 6701aac on 8.x-1.x 
    Issue #2860279 by josephdpurcell, nikathone, bojanz: Multiple state...
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs work » Fixed

Rewrote the patch and committed. Thanks, everyone!

sittard’s picture

sittard CreditAttribution: sittard commented

We are currently using State Machine 8.x-1.0-beta3 and we need this patch. Are we OK to migrate to the dev branch or should we wait for a beta 4 release? Thanks.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

Released RC1, you can update to that.

sittard’s picture

sittard CreditAttribution: sittard commented

Super - Thanks ;)

Status: Fixed » Closed (fixed)

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