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.
Comment | File | Size | Author |
---|---|---|---|
#13 | 2809489-13.patch | 5.03 KB | ApacheEx |
Comment | File | Size | Author |
---|---|---|---|
#13 | 2809489-13.patch | 5.03 KB | ApacheEx |
Comments
Comment #2
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI created a partial conversion and am in doubt about it.
While converting I saw that the JS part of this test is testing drag-n-drop. There are many tests that test this already, in particular #2809483: Convert AJAX part of \Drupal\field\Tests\FormTest::testFieldFormJSAddMore to JavascriptTestBase.
The conversion can still be usefull as the test is a little less hacky than the WebTest but for now no JS is tested.
To be decided:
Should this test contain validation for the drag-n-drop?
If not:
- Should the current js-add-more test be extended to use multiple draggable fields?
- Should the current conversion be committed or only a patch stripping the Add more and draggable test part?
Comment #3
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #4
dawehnerAs said in the parent issue, I think we should keep the conversions as parallel as possible.
Comment #5
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThanks for answering my question.
I'll add the drag-n-drop functionality and update the issue with a new patch.
Comment #6
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI updated the test to be an exact copy of the original, testing the dragging and add more behavrior. As a bonus the expanding of the second entity is also tested, this was required because the dragging and add more functionality couldn't be tested otherwise..
Comment #7
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedUpdate to conform to the new wait API.
Comment #9
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #10
dawehnerJust some quick general review.
This needs some docs
If its a trait we could use proper function names / protected visibility and then wire it up to the old names in the test files.
Let's use a fully qualified class name (FQCN)
Some more visibility.
Comment #11
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #13
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Internetdevels, Drupal Ukraine Community commentedI think it's better to convert whole
NestedFormTest
with minimal changes to BTB.there is no interdiff because it's completely a new patch.
one thing to highlight:
instead of this
I use this
I don't know how it worked before,
changed
field will be the same ascreated
after first save like this:here is a proof: \Drupal\Core\Field\Plugin\Field\FieldType\ChangedItem::preSave()
Comment #16
LendudeThe use of AJAX here was as a workaround for limitations in WebTestBases drupalPostForm and we have proper coverage for the 'add more' javascript in
\Drupal\Tests\field\FunctionalJavascript\FormJSAddMoreTest::testFieldFormJsAddMore
. This seems sufficient for this, I think.So just converting this to BrowserTestBase makes sense here I think.
The conversion looks good, nice and minimal.
Updated the IS to reflect the new direction.
Comment #17
alexpottCan someone update the issue summary - I've updated the title but now the issue summary is out-of-date. Once the issue summary is updated it can be set back to rtbc.
Comment #18
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Drupal Ukraine Community commentedUpdated summary.
Comment #19
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Drupal Ukraine Community commentedComment #20
alexpottCommitted and pushed c93ced1ea2 to 8.7.x and 9082b8de9b to 8.6.x. Thanks!