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

Comments

nikathone created an issue. See original summary.

nikathone’s picture

nikathone
Primary language Swahili
Location Ontario
commented
Status: Active » Needs review
StatusFileSize
new 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
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 commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new state_machine-multiple-state-transition-form-same-page-form-id-conflict-2860279-4.patch1.08 KB
new 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 commented
Issue summary: View changes
StatusFileSize
new 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 commented

#4 is working great!

uniquename’s picture

uniquename commented

works fine for me!

bojanz’s picture

bojanz 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 commented
Status: Needs work » Fixed

Rewrote the patch and committed. Thanks, everyone!

sittard’s picture

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 commented

Released RC1, you can update to that.

sittard’s picture

sittard commented

Super - Thanks ;)

Status: Fixed » Closed (fixed)

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