Discovered in #1808220: Remove run-tests.sh dependency on existing/installed parent site
triggering_element.triggering_element is used for #limit_validation_errors.This causes programmed form submissions of the install_settings_form() to throw validation errors, because the alternative database driver sections are not ignored.
For programmed form submissions, if no triggering_element was identified, and if only one button exists in the form, automatically use this button as triggering_element.
FormBuilder already contains code to check whether a button element is the triggering element, but all of that code depends on a form value to be submitted for the button element. — If there is no submitted form value, then none of the existing code is triggered.$form array recursion, which means that not all possible buttons have been collected yet.| Comment | File | Size | Author |
|---|---|---|---|
| #8 | form_submission-2214055-8.patch | 2.97 KB | dawehner |
| #2 | form.triggering.2.patch | 4.42 KB | sun |
| #1 | drupal8.form-triggering-element-programmed.1.patch | 1.17 KB | sun |
| drupal8.form-triggering-element-programmed.0.patch | 1.07 KB | sun |
Comments
Comment #1
sunAs an attempt to avoid any doubts, let's limit that to forms that have exactly one submit button.
Comment #2
sunNow including test coverage.
Btw, this is how most of our form test coverage should look like.
Comment #3
sunClarified issue summary. Would be great to move forward here, since this essentially blocks #1808220: Remove run-tests.sh dependency on existing/installed parent site
Comment #4
moshe weitzman commentedLooks reasonable, and has a nice test.
Comment #5
webchickCommitted and pushed to 8.x. Thanks!
Comment #6
dawehnerNice trick with the test class. Is there any reason we could not simply extend the existing unit test here?
Comment #7
sun@dawehner: Which existing do you mean? I searched through all the existing tests, but all of them used a form with multiple submit buttons and other gimmicks.
In addition, I really wanted to get a first incarnation of this new/proper approach for (drupal-)unit-testing form processing into core. — IMO, we should convert all of the other form related tests into separate test classes, following this concept; i.e., exactly one form + test case per test class. The class can have multiple test methods, but only one form.
That is, because this concept encapsulates the entire test code into a single class/file. So you don't have to look up the actual form code in a completely other spot of the code base anymore. Doing so might allow us to get rid of the form_test.module.
Comment #8
dawehnerYou can do the same with phpunit tests, pretty great actually.
Comment #9
sunUgh. I wasn't aware of that FormBase phpunit test class. — That code looks extremely scary to me. It mocks way too many aspects.
→ We're not testing the actual form processing code, so those tests are borderline useless.
Especially a security related test like this has to be bullet-proof and we must be sure that the actual code behaves in the exact identical way.
In fact, form processing is almost exclusively about security. That
FormBaseclass mocks all of the form element #types, plenty ofFormBuildermethods, and a lot more. None of that reflects the actual situation.I'm very scared now — I can only hope we're not using this for essential form processing tests.
Comment #10
dawehnerWell, we mock away the external dependencies, this is a good practise.
Comment #11
tim.plunkettWe're not testing the actual form processing code, so those tests are borderline useless.Which code? And which part is useless?
Comment #12
sunThe triggering_element detection code of Form API depends on the internal properties of #type submit, #type button, and #type image_button.
#type submit is mocked (duplicated) - but incompletely - in
FormBase::getElementInfo().Other form element #types do not exist at all. #type form itself does not exist to begin with.
As a consequence, any test that is based on
FormBasedoes not and cannot test the actual form processing functionality. That phpunit base class might be useful for simple behavioral tests ofFormBuilderitself (to assert thatfoo()calls intobar()), but using it for any test that actually deals with form elements (of any type) and/or their processing is a non-starter.To be crystal clear, every form processing and form element processing test that uses this class is a malformed test, because fundamental concepts of Form API, including security measures, are explicitly mocked to effectively disable them. Element type specific processing simply does not occur. That defeats the one and only purpose of form processing tests.