Uncovered over at #582584: form_required_marker() isn't passed the form element (theme_hook definition is wrong). I couldn't fix this via #582956: FormsTestCase::testRequiredFields() is broken in various ways since we weren't trying to drupal_render() at that point. DamZ says (and I believe him) that these tests are all wrong, and we shouldn't be doing the unholy things were doing in a failing attempt to unit test FAPI like this. Instead of doing the full form build/render/submit cycle, we're trying to just do pieces of it, and I believe that's what's causing all the PHP notices when you attempt to call drupal_render() on a FAPI form element of type 'radios' that's only been partially built and processed.
For now, #582584 just doesn't attempt to test if the required marker shows up on the radios. I really don't feel like completely rewriting the FAPI tests for what started as a trivial FAPI patch that mushroomed into a multi-day effort. :/
chx said he was okay with #582584 if I created this issue and reference it in an @todo code comment. So, here's the issue.
Someone, someday, should either rewrite these tests to do the full form lifecycle, or they should debug and figure out why attempting to render the bogus valued radios results in PHP notices.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 588438-12.patch | 1.18 KB | dww |
Comments
Comment #1
effulgentsia commentedsubscribing to look at later.
Comment #2
mlncn commentedA functional test for radio buttons is part of #811542: Regression: Required radios throw illegal choice error when none selected.
Comment #3
sunI just discovered this code/hack in #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes.
The entire test class should be converted to be based on
DrupalUnitTestBase.Comment #12
dwwAs part of my #BugSmash homework to triage bug reports I opened, re-found this issue. ;)
There's still a @todo for the work-around still in the test. Happily, Drupal\Tests\system\Functional\Form\FormTest still passes with the hack removed. So the original problems have since been solved and we can remove the tech debt in the test.
Thanks,
-Derek
p.s. Queuing for testing on 9.2.x, but it applies back to 8.9.x if we want it that far...
Comment #13
longwaveNice find, we must have refactored something over the years that means this works correctly now!
Comment #14
alexpottCommitted and pushed 4731e3c6de to 9.3.x and bf341805c8 to 9.2.x. Thanks!
Comment #17
dwwYay, thanks! Great to see a 2009 bug smashed. 😀
Cheers,
-Derek