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.In user_filter_form_submit(), we validate that the user selected a valid option. I believe this is unnecessary, as the Form API already does this validation for us.
function user_filter_form_submit($form, &$form_state) {
// ...
// Apply every filter that has a choice selected other than 'any'.
foreach ($filters as $filter => $options) {
if (isset($form_state['values'][$filter]) && $form_state['values'][$filter] != '[any]') {
// Merge an array of arrays into one if necessary.
$options = ($filter == 'permission') ? form_options_flatten($filters[$filter]['options']) : $filters[$filter]['options'];
// Only accept valid selections offered on the dropdown, block bad input.
if (isset($options[$form_state['values'][$filter]])) {
$_SESSION['user_overview_filter'][] = array($filter, $form_state['values'][$filter]);
}
}
}
The validation code dates to the original introduction of the user filtering functionality in #34505: admin/user filter.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | remove_unnecessary_validation-1315008-6.patch | 2.48 KB | bfroehle |
| #5 | user_filter_validation-1315008.patch | 1.43 KB | bfroehle |
| #2 | 1315008-with-patch.png | 60.18 KB | bfroehle |
| #1 | user_filter_validation-1315008.patch | 1.4 KB | bfroehle |
Comments
Comment #1
bfroehle CreditAttribution: bfroehle commentedPatch attached.
Comment #2
bfroehle CreditAttribution: bfroehle commentedFor the curious, I've attached the result of submitting an invalid item after applying this patch --- clearly the Form API is catching the invalid selection.
(The error, incidentally is the same without the patch applied, showing that the relevant sections of code never get called).
Comment #3
xjmLooks sane to me. Is there already a test for this behavior?
Comment #4
xjmTagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #5
bfroehle CreditAttribution: bfroehle commentedComment #6
bfroehle CreditAttribution: bfroehle commentednode.module has a similar section of code which is also unnecessary! Adding that hunk into the patch as well.
Comment #7
xjmNice! Two thoughts:
In the process of thinking about what the test should be, I realized the "illegal choice" error message is probably not correct.Edit: Never mind. It is actually the correct error here. It's just not the most informative help text, but that's a separate issue. :)Comment #8
bfroehle CreditAttribution: bfroehle commented1. There's already test coverage for the page. See testUserAdmin. The removed sections of code cannot be tested for as an illegal choice is already detected in http://api.drupal.org/api/drupal/includes--form.inc/function/_form_valid...
Comment #9
xjmWhat I'm saying is there are options that don't get selected in the test. The test just filters by status and then refines by type, so the particular path that the redundant code is for is never tested. There's no test that would fail if the redundant code weren't redundant. Edit: I'm referring to
NodeAdminTestCase::testContentAdminPages(); I didn't look at the user form test yet.Comment #10
bfroehle CreditAttribution: bfroehle commentedAnd generically form.test already checks against input forgery (which I guess is what this code is trying to prevent).
Comment #11
xjmHm, alright. :)
Comment #13
catchI had a quick look in git to see where this was added but didn't come up with much for these specific hunks, however it looked like pre-FAPI to me. Amazing how much pre-FAPI code is still in core. Committed/pushed to 8.x.
Comment #14
bfroehle CreditAttribution: bfroehle commentedAs far as I can tell the code originated in #34505: admin/user filter and #29465: New drupal forms api. -- see 30a322e and 7e1527, for example.