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.
Found in #1381140: A #default_value = 0 for #type radios checks all radios
\Drupal\workflows\Form\WorkflowTransitionAddForm::form sets a default value for radios as an empty array. Radios can never have an array value, there can only be one value checked.
Comment | File | Size | Author |
---|---|---|---|
#10 | 2900911-10.patch | 1.54 KB | scott_euser |
#10 | 2900911-7-10.txt | 1 KB | scott_euser |
#6 | 2900911-6.patch | 1.42 KB | Sam152 |
#6 | interdiff.txt | 954 bytes | Sam152 |
#4 | 2900911-4.patch | 1.42 KB | scott_euser |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedIf we don't want anything selected by default, we can just remove this right?
Comment #3
Lendude@Sam152 yeah that should do it.
Dug around a bit but doesn't seem to be test coverage for this field having nothing selected by default.
We should probably add a line to
\Drupal\Tests\workflows\Functional\WorkflowUiTest
This seems to be all the coverage the form has at the moment, or is there some more somewhere else?
Comment #4
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedI think that `hasCheckedField()` would work for that. Does the attached look okay?
Comment #6
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis oughta do it.
Comment #7
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedAh missed the session! Was about to submit this
$this->assertFalse($this->getSession()->getPage()->hasCheckedField('to'));
But I see your checkboxNotChecked() also works for radios in the same way
Comment #8
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedComment #9
LendudeThe problem with checkboxNotChecked (aside from the confusing use on a radio button) only checks the first element with the given name (it uses find(), not findAll()). Just as a quick test, if you change the default for 'to' to 'draft', this test still passes ('published' is the first radio button for 'to').
I would go for something like:
Comment #10
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedRight makes sense, thanks! Took me a bit to get phantomjs working in my containers so wasn't able to test for a bit, but got it going now. Updated the patch with your above suggestion which worked for me in testing.
Comment #12
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedTests failure was Jenkins not able to access db. Reran and all passed.
Comment #13
LendudeLooks great now. Thanks!
Comment #14
larowlanUpdating issue credit to add @Lendude who provided reviews that shaped the final patch.
Comment #15
larowlanCommitted as 3bbf314 and pushed to 8.5.x
Cherry-picked as deaa514 and pushed to 8.4.x