Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2783685-25-test-only.patch | 29.26 KB | samuel.mortenson |
#25 | interdiff-15-25.txt | 249 bytes | samuel.mortenson |
#25 | 2783685-25.patch | 31.11 KB | samuel.mortenson |
Comments
Comment #2
Leksat CreditAttribution: Leksat at Amazee Labs commentedComment #3
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedLooks good. Only one thing:
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.
Comment #4
samuel.mortensonI'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?
Comment #5
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedI don't think that there is. And we want to test Paragraphs integration anyway.
Comment #6
amoebanath CreditAttribution: amoebanath at ComputerMinds commentedJust 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 :)
Comment #7
samuel.mortensonFinally made time to circle back to this, working on test coverage now.
Comment #8
samuel.mortensonNot an easy one to write! If TestBot passes I'll upload a test-only patch to show that the coverage is good.
Comment #11
samuel.mortensonLet's try it again without the explicit entity_reference_revisions test dependency.
Comment #14
samuel.mortensonFixing the IEF tests now, can't replicate the Paragraphs failure.
Comment #15
samuel.mortensonComment #18
samuel.mortensonHere's a funny patch which should let testbot tell us the real error.
Comment #19
samuel.mortensonComment #24
samuel.mortensonSo the real test failure is:
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?
Comment #25
samuel.mortensonJust remembered that I'm a maintainer and can commit small changes like adding test dependencies, hah. Let's try the tests again.
Comment #26
samuel.mortensonTests are green! Here is the test only patch to finish my comment stampede.
Comment #29
samuel.mortensonComment #30
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedWow! Amazing! Committed.
Thank you all! Time for another alpha release (possibly last before beta?).