Problem/Motivation

When user loads an entity browser form we load generate UUID of a so-called entity browser instance. This UUID allows us to differentiate between multiple instances on the same page.

Some display plugins need UUID information even before the form was generated (in DisplayInterface::displayEntityBrowser()). It seems that we'll need to generate UUID earlier in some cases.

Proposed resolution

One solution was proposed until now - better solutions welcome. We could generate UUID in DisplayInterface::displayEntityBrowser() and pass it on to the form via URL argument. Form would then check if the UUID already exists and generate it only if not.

Original issue summary

In both the Modal and IFrame Display plugins, the REGISTER_JS_CALLBACKS event is dispatched and RegisterJSCallbacks objects are created without passing the required $instance_uuid argument. I would submit a patch but I'm not sure where we can get an instance UUID for the Entity Browser (I believe it's abstracted from the Display Plugin). The relevant calls are in Modal::displayEntityBrowser() and IFrame::displayEntityBrowser().

CommentFileSizeAuthor
#17 interdiff.txt2.08 KBslashrsm
#17 2600706_17.patch17.14 KBslashrsm
#16 interdiff.txt2.57 KBslashrsm
#16 2600706_16.patch16.42 KBslashrsm
#14 2600706_14.patch15.46 KBslashrsm
#11 2600706_11.patch15.45 KBslashrsm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuel.mortenson created an issue. See original summary.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

Submitted https://github.com/drupal-media/entity_browser/pull/107

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented

Instance UUID is now stored in form state. See: https://github.com/drupal-media/entity_browser/blob/8.x-1.x/src/Form/Ent...

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

As the Events::REGISTER_JS_CALLBACKS event is dispatched before the form is rendered, what's the best way to handle this? Pass another generated UUID?

rackberg’s picture

rackberg CreditAttribution: rackberg as a volunteer commented

Maybe this will fix it: https://github.com/drupal-media/entity_browser/pull/110

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

That suffers the same problem if I understand correctly - the UUID you're passing isn't the instance_uuid (which is generated in the form).

slashrsm’s picture

slashrsm CreditAttribution: slashrsm as a volunteer commented

Looking at the code it seems that we don't even read the UUID property anywhere. Could we just remove it from event constructor?

Even if we do that we still have problem of two different UUIDs for a given instance (one generated in display plugin and one generated in form class). General code flow usually goes something like this:

1. EB is loaded and DisplayInterface::displayEntityBrowser() is called to get initial representation of EB.
2. User interacts with initial representation.
3. Actual EB form is build and displayed to the user. When this first happens form class generated UUID.
4. ...

We could change step 4 a bit. It could check if UUID was passed through URL and use that instead of generating new one. This would re-use UUID that was generated in DisplayInterface::displayEntityBrowser() and also work for display that don't generate it (standalone).

How does that sound? Is there any security aspect of this that we should consider.

  • slashrsm committed 21d6b49 on 8.x-1.x 
    Issue #2600706 by slashrsm: Explain temporary fix with an additional...
slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
Title: $instance_uuid not passed to RegisterJSCallbacks::__construct() in built in Displays » Instance UUID is sometimes needed even before then entity browser form is built
Category: Bug report » Task
Issue summary: View changes
Issue tags: +D8Media, +Media Initiative

I decided to merge #2 as it fixes fatal. Added a comment to explain that is a temp change and linked it back to this issue: https://github.com/drupal-media/entity_browser/commit/21d6b493818d86b839...

Leaving this open until we resolve issues from #7.

Updating issue summary.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

We could change step 4 a bit. It could check if UUID was passed through URL and use that instead of generating new one. ...

I think this is a good option, providing the instance UUID early on in the process so that the Browser and Form can take advantage of it. On the other hand, if we don't actually know why we're using an instance UUID in the first place, we might consider removing it.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
Status: Active » Needs review
FileSize
2600706_11.patch15.45 KB

Here we go.

Status: Needs review » Needs work

The last submitted patch, 11: 2600706_11.patch, failed testing.

The last submitted patch, 11: 2600706_11.patch, failed testing.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
Status: Needs work » Needs review
FileSize
2600706_14.patch15.46 KB
thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs review » Needs work 
  1. +++ b/src/DisplayBase.php
@@ -40,6 +37,20 @@ abstract class DisplayBase extends PluginBase implements DisplayInterface, Conta
+  /**
+   * Instance UIID string.
+   *

    Typo. 

  2. +++ b/src/Form/EntityBrowserForm.php
@@ -43,7 +43,12 @@ class EntityBrowserForm extends FormBase implements EntityBrowserFormInterface {
+    if (empty($_GET['uuid'])) {
...
+      $form_state->set(['entity_browser', 'instance_uuid'], $_GET['uuid']);

    Hmm, is there no better way to get uuid then using superglobal? 

  3. +++ b/src/Form/EntityBrowserForm.php
@@ -43,7 +43,12 @@ class EntityBrowserForm extends FormBase implements EntityBrowserFormInterface {
+      $form_state->set(['entity_browser', 'instance_uuid'], \Drupal::service('uuid')->generate());

    Can we inject this

We can remove contains in Drupal\entity_browser\Plugin\EntityBrowser\Display\Standard

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
Status: Needs work » Needs review
FileSize
2600706_16.patch16.42 KB
interdiff.txt2.57 KB
slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
FileSize
2600706_17.patch17.14 KB
interdiff.txt2.08 KB

Injected uuid.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Looks great.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
Status: Reviewed & tested by the community » Fixed

  • slashrsm committed b716629 on 8.x-1.x 
    Issue #2600706 by slashrsm, samuel.mortenson, Denchev, rackberg:...

Status: Fixed » Closed (fixed)

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