Problem/Motivation

NoDisplay selection display does:

  public function submit(array &$form, FormStateInterface $form_state) {
      $this->selectionDone($form_state);
  }

This means that every time an entity browser that uses this display is submitted the selection will be propagated. This works OK for single-step widgets where we can safely assume that submit also means that we completed the selection.

The problem becomes visible when using multi-step workflow which expects for selection to only be propagated at the end of the multi-step process.

Proposed resolution

Check if the main submit button was used to submit the form and only propagate the selection if this is the case.

Remaining tasks

- propose patch
- add test coverage

CommentFileSizeAuthor
#9 2842411_8.patch6.2 KBslashrsm
#9 interdiff.txt4.7 KBslashrsm
#9 2842411_8_TEST_ONLY.patch5.45 KBslashrsm
#7 nodisplay_selection-2842411-7.patch6.98 KBginovski
#7 interdiff-2842411-4-7.txt5.05 KBginovski
#7 javascript-test-widget.png43.29 KBginovski
#4 nodisplay_selection-2842411-4.patch6.95 KBginovski
#2 2842411_2.patch771 bytesslashrsm

Comments

slashrsm created an issue. See original summary.

slashrsm’s picture

slashrsm commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 2842411_2.patch771 bytes
ginovski’s picture

ginovski commented
Assigned: Unassigned » ginovski
ginovski’s picture

ginovski commented
StatusFileSize
new nodisplay_selection-2842411-4.patch6.95 KB

1. Created test widget with multiple submit buttons.
2. Added default configuration for the widget.
3. Added test method in EntityBrowserUITest

Status: Needs review » Needs work

The last submitted patch, 4: nodisplay_selection-2842411-4.patch, failed testing.

slashrsm’s picture

slashrsm commented 
  1. +++ b/src/Tests/EntityBrowserUITest.php
@@ -68,4 +71,63 @@ class EntityBrowserUITest extends WebTestBase {
+  public function testMultipleSubmitWidget() {

    I'd prefer if this test would be a javascript test. We have plenty of them that are doing similar things which should be a good example. 

  2. +++ b/src/Tests/EntityBrowserUITest.php
@@ -68,4 +71,63 @@ class EntityBrowserUITest extends WebTestBase {
+      'access entity_browser_test_entity_form entity browser pages',

    This permissions should be of the "access [entity_browser_id] browser pages" form. You should only need one. 

  3. +++ b/src/Tests/EntityBrowserUITest.php
@@ -68,4 +71,63 @@ class EntityBrowserUITest extends WebTestBase {
+      'create content',

    This permission does not exist. 

  4. +++ b/src/Tests/EntityBrowserUITest.php
@@ -68,4 +71,63 @@ class EntityBrowserUITest extends WebTestBase {
+    // Click the second submit button to make sure the widget does not close.
+    $this->drupalPostForm('NULL', [], 'Second submit button');

    You need to step into iFrame in order to test things inside entity browser. There are many examples in the module. 

  5. +++ b/src/Tests/EntityBrowserUITest.php
@@ -68,4 +71,63 @@ class EntityBrowserUITest extends WebTestBase {
+    // Check that the entity browser widget is still open.
+    $this->assertText('Select entities');

    You can add some dummy text to the widget and also check for the presence of that. 

  6. +++ b/src/Tests/EntityBrowserUITest.php
@@ -68,4 +71,63 @@ class EntityBrowserUITest extends WebTestBase {
+    $this->drupalPostForm('Select entities');

    Missing required argument.

ginovski’s picture

ginovski commented
StatusFileSize
new javascript-test-widget.png43.29 KB
new interdiff-2842411-4-7.txt5.05 KB
new nodisplay_selection-2842411-7.patch6.98 KB

Addressed #6, now a javascript method.
Tested it locally, works, not sure that test bot is also running js tests.

First asserting when clicking the second submit, that the second button is still visible (implicit check that the widget is open)
Then asserting when clicking the primary submit, that the second button is not visible (implicit check that the widget is closed).

ginovski’s picture

ginovski commented
Status: Needs work » Needs review
slashrsm’s picture

slashrsm commented
StatusFileSize
new 2842411_8_TEST_ONLY.patch5.45 KB
new interdiff.txt4.7 KB
new 2842411_8.patch6.2 KB

Moved the test to a different, more appropriate, test.

The last submitted patch, 9: 2842411_8_TEST_ONLY.patch, failed testing.

  • slashrsm committed 7e5bf4f on 8.x-1.x 
    Issue #2842411 by slashrsm, Ginovski: NoDisplay selection display plugin...
slashrsm’s picture

slashrsm commented
Status: Needs review » Fixed
Issue tags: -Needs tests

Committed. Thanks!

Status: Fixed » Closed (fixed)

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