In EntityReferenceBrowserWidget we have this line:

'#name' => $this->fieldDefinition->getName() . '_remove_' . $entity->id() . '_' . $row_id,

There could be cases when we can end up with several remove buttons having the same name. And this will make Drupal unable to determine the triggering element.

Example entity:

field_paragraphs
  paragraph 1
    field_reference targeting node 1
  paragraph 2
    field_reference targeting node 1

Both Remove buttons will have "field_reference_1_0" name in this case.

The same is true for FileBrowserWidget.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Leksat created an issue. See original summary.

Leksat’s picture

Assigned: Leksat » Unassigned
Status: Active » Needs review
FileSize
1.89 KB
slashrsm’s picture

Issue tags: +D8Media, +Needs tests

Looks good. Only one thing:

- '#name' => $this->fieldDefinition->getName() . '_remove_' . $entity->id() . '_' . $row_id,
+ '#name' => $this->fieldDefinition->getName() . '_remove_' . $entity->id() . '_' . $row_id . '_' . implode(':', $field_parents),

implode(':', $field_parents) could get very long. Could we use its hash instead?

This will need tests, but we can commit patch first and then work on tests.

samuel.mortenson’s picture

FileSize
1.84 KB

I've added the hashing suggested in #3 (read http://stackoverflow.com/a/7723730 for source of md5 call).

For tests, is there a way to replicate this in core without something like Paragraphs?

slashrsm’s picture

Priority: Major » Normal
Status: Needs review » Needs work

I don't think that there is. And we want to test Paragraphs integration anyway.

amoebanath’s picture

Just a note to say that I have run into this exact issue in the last few days, particularly with nested paragraphs.
Patch #4 does seem to resolve it nicely :)

samuel.mortenson’s picture

Finally made time to circle back to this, working on test coverage now.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
30.35 KB

Not an easy one to write! If TestBot passes I'll upload a test-only patch to show that the coverage is good.

Status: Needs review » Needs work

The last submitted patch, 8: 2783685-8.patch, failed testing.

The last submitted patch, 8: 2783685-8.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
30.29 KB
341 bytes

Let's try it again without the explicit entity_reference_revisions test dependency.

Status: Needs review » Needs work

The last submitted patch, 11: 2783685-11.patch, failed testing.

The last submitted patch, 11: 2783685-11.patch, failed testing.

samuel.mortenson’s picture

Fixing the IEF tests now, can't replicate the Paragraphs failure.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
31.45 KB
1.03 KB

Status: Needs review » Needs work

The last submitted patch, 15: 2783685-15.patch, failed testing.

The last submitted patch, 15: 2783685-15.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
32.3 KB

Here's a funny patch which should let testbot tell us the real error.

samuel.mortenson’s picture

FileSize
32.05 KB

The last submitted patch, 18: 2783685-debug-1.patch, failed testing.

The last submitted patch, 18: 2783685-debug-1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2783685-debug-2.patch, failed testing.

The last submitted patch, 19: 2783685-debug-2.patch, failed testing.

samuel.mortenson’s picture

So the real test failure is:

1) Drupal\Tests\entity_browser\FunctionalJavascript\ParagraphsTest::testParagraphs
Drupal\Core\Extension\MissingDependencyException: Unable to install modules: module 'entity_browser_test_paragraphs' is missing its dependency module paragraphs.

I added paragraphs to the test_dependencies in 2783685-15.patch, any ideas as to why the TestBot wouldn't be downloading it?

I found #2692407: Test_dependencies are downloaded before applying patches, rather than after, which implies that patches can't add test dependencies. If that's true, can we commit the test dependency for Paragraphs and re-roll this patch?

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
31.11 KB
249 bytes

Just remembered that I'm a maintainer and can commit small changes like adding test dependencies, hah. Let's try the tests again.

samuel.mortenson’s picture

FileSize
29.26 KB

Tests are green! Here is the test only patch to finish my comment stampede.

Status: Needs review » Needs work

The last submitted patch, 26: 2783685-25-test-only.patch, failed testing.

The last submitted patch, 26: 2783685-25-test-only.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
slashrsm’s picture

Status: Needs review » Fixed

Wow! Amazing! Committed.

Thank you all! Time for another alpha release (possibly last before beta?).

Status: Fixed » Closed (fixed)

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