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.\Drupal\views_ui\ViewListBuilder::render() builds a render array for the ViewListBuilder.
The local variable used to hold this render array is named $list
But there is one element, and only one element, that is put into the $form array. The $form variable is not defined or used anywhere else, so this is dead, non-functioning code. Here's an excerpt:
$list['#type'] = 'container';
$list['#attributes']['id'] = 'views-entity-list';
$list['#attached']['library'][] = 'core/drupal.ajax';
$list['#attached']['library'][] = 'views_ui/views_ui.listing';
$form['filters'] = [ // <--- Uses $form instead of $list
'#type' => 'container',
'#attributes' => [
'class' => ['table-filter', 'js-show'],
],
];
$list['filters']['text'] = [
'#type' => 'search',
'#title' => $this->t('Filter'),
'#title_display' => 'invisible',Putting the search form element into a container was a change was made a long time ago in #2087327-20: [Regression from D7] Add text search functionality to views listing page. Apparently no-one ever noticed that patch didn't do what it was supposed to do, as a result of using $form instead of $list.
The attached patch fixes that mistake by changing $form to $list, and consequently adds the functionality originally intended by #2087327-20: [Regression from D7] Add text search functionality to views listing page.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | Screen Shot 2019-04-12 at 11.21.43 AM.png | 192.33 KB | kimberlydb |
| #10 | Screen Shot 2019-04-12 at 11.21.13 AM.png | 179.88 KB | kimberlydb |
| views-list-builder.patch | 575 bytes | TR | |
Comments
Comment #2
TR CreditAttribution: TR commentedComment #3
TR CreditAttribution: TR commentedComment #4
pratik.mehta19 CreditAttribution: pratik.mehta19 as a volunteer commentedComment #6
angel.hI think that changing the code like this is not a good idea. This might cause a regression given the fact that this code has been like this from
8.0.x-dev. I think the safest thing would be to just remove the variable.
Comment #7
angel.hI also think we need tests to check that the initial functionality did not change.
Comment #8
TR CreditAttribution: TR commentedMarkup before the patch:
Markup after the patch:
That's it. One stand-alone form element wrapped in an extra div. Nothing removed.
The JavaScript attached to that form element continues to work because the existing classes haven't been removed so the selectors all remain valid. @dawehner confirmed this in #2087327-21: [Regression from D7] Add text search functionality to views listing page.
This form element exists only for use by the JavaScript; it is not involved in the form validation or submission. Nothing else on the page is or can be affected by this change and there's nothing additional to test that isn't already tested on that page. Unless you REALLY think we should test to ensure the js-show class shows up on that page, which isn't something we do ANYWHERE else in core.
The purpose of adding this new div is stated by @tstoeckler in #2087327-20: [Regression from D7] Add text search functionality to views listing page:
Those two other pages he mentions both use exactly the structure restored in this patch:
@see \Drupal\system\Form\ModulesListForm
@see \Drupal\simpletest\Form\SimpletestTestForm
@tstoeckler, @dawehner, and @webchick all approved of this change in #2087327: [Regression from D7] Add text search functionality to views listing page.
I really don't think we need to re-litigate this.
Comment #10
kimberlydb CreditAttribution: kimberlydb as a volunteer commentedVerified this is the minor markup change and filtering still works as expected.
Before & after screenshots attached.
Comment #11
alexpottComment #12
alexpottI think changing this markup is okay - it is ViewsUI markup and therefore not markup for the normal use case and it is only for the javascript filter on the page. It also fixes the page for the non-js usecase so I'm not taking #6 into account. I thought about #7 and requiring tests but I'm not sure that testing this is really that important and I'm also not how to test the functionality without asserting against the markup which doesn't feel that valid.
But also because this is a markup change and only a minor bugfix I'm going to commit only to 8.8.x and give anyone who is relying on the current markup sometime to change their code to support this as well. I do feel this is exceptionally unlikely but with Drupal that someone relies of something extremely edge proves to be the norm so no harm in being cautious.
Committed f7aeba5 and pushed to 8.8.x. Thanks!