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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lendude created an issue. See original summary.

Sam152’s picture

Status: Active » Needs review
FileSize
553 bytes

If we don't want anything selected by default, we can just remove this right?

Lendude’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

@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->clickLink('Add a new transition');
    $this->submitForm(['id' => 'publish', 'label' => 'Publish', 'from[draft]' => 'draft', 'to' => 'published'], 'Save');
    $this->assertSession()->pageTextContains('Created Publish transition.');
    $workflow = $workflow_storage->loadUnchanged('test');
    $this->assertTrue($workflow->getTypePlugin()->getState('draft')->canTransitionTo('published'), 'Can transition from draft to published');

    $this->clickLink('Add a new transition');
    $this->submitForm(['id' => 'create_new_draft', 'label' => 'Create new draft', 'from[draft]' => 'draft', 'to' => 'draft'], 'Save');
    $this->assertSession()->pageTextContains('Created Create new draft transition.');
    $workflow = $workflow_storage->loadUnchanged('test');
    $this->assertTrue($workflow->getTypePlugin()->getState('draft')->canTransitionTo('draft'), 'Can transition from draft to draft');

This seems to be all the coverage the form has at the moment, or is there some more somewhere else?

scott_euser’s picture

Status: Needs work » Needs review
FileSize
755 bytes
1.42 KB

I think that `hasCheckedField()` would work for that. Does the attached look okay?

Status: Needs review » Needs work

The last submitted patch, 4: 2900911-4.patch, failed testing. View results

Sam152’s picture

Status: Needs work » Needs review
FileSize
954 bytes
1.42 KB

This oughta do it.

scott_euser’s picture

Ah 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

scott_euser’s picture

Lendude’s picture

+++ b/core/modules/workflows/tests/src/Functional/WorkflowUiTest.php
@@ -137,7 +137,7 @@ public function testWorkflowCreation() {
+    $this->assertSession()->checkboxNotChecked('to');

The 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:

    $this->assertCount(2, $this->cssSelect('input[name="to"][type="radio"]'));
    $this->assertCount(0, $this->cssSelect('input[name="to"][checked="checked"][type="radio"]'));
scott_euser’s picture

Right 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.

Status: Needs review » Needs work

The last submitted patch, 10: 2900911-10.patch, failed testing. View results

scott_euser’s picture

Status: Needs work » Needs review

Tests failure was Jenkins not able to access db. Reran and all passed.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Looks great now. Thanks!

larowlan’s picture

Updating issue credit to add @Lendude who provided reviews that shaped the final patch.

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 3bbf314 and pushed to 8.5.x

Cherry-picked as deaa514 and pushed to 8.4.x

Status: Fixed » Closed (fixed)

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