Add support for tables with two headers in views table display

tidying up the issue summary

\template_preprocess_views_view_table would be probably the right place to add it.

The tables currently generated by views are all tables with one header.

This isn't really a bug in my view, in that I don't think we should force authors to use a two-header table model. Looking though the WAI table concepts tutorial, it's clear that tables with one header are quite acceptable for many cases, including many of the administrative views in Drupal core.

Instead, let's treat this as a feature request. It would be really great if views could support some of the other table concepts described there!

I think it's feasible to support simple tables with two headers. The table display plugin could have an extra setting to say that a particular field is treated as a row header, and scope="row" could be added via the column.attributes twig variable (I think). This would be flexible enough to support tables with an offset column of header cells. I'm not sure what to do about the top-left cell, which might need to be blank.

Support for some tables with irregular headers might be possible, perhaps using views query grouping, but would add more configuration options. This might be best explored as a separate views display plugin provided by a contrib module.

First, I'd like to agree what Andrew said & disagree with this "Accessible tables have scope="col" in the headers and scope="row" in the first field in a row." It all depends on the table and the data you are trying to display. Right now you can't define the scope="row" which is a problem if have a heading there you want to use.

I'm fine with looking ahead at how to make the Views tables more complex. There is a lot of tabular data that I don't think we can very effectively show right now.

Right now we generally have the first row being headings and all the other cells being data cells. I don't know of an instance where the first field in each table row is actually a heading.

Sometimes it is a checkbox. I guess if it is a first name or title of an article that could be considered a heading.

There are great examples of accessible table structures here https://www.w3.org/WAI/tutorials/tables/

I just don't know how we would get to two meaningful headers in Views table display so that it reflects the needs of https://www.w3.org/WAI/tutorials/tables/two-headers/

I don't see where gettysburger uploaded a patch for this on 8.

I created a sandbox project for this issue on 7: Views Table Row Headers. I'm reasonably experienced site builder, but I'm very new to drupal development. If someone would be willing to mentor me either through releasing this as a module and porting it to 8 or making the change to core in 8, I would be very grateful.

Attempting a patch for d8.

Cleaned up issue summary.

Can I configure this through Views UI? Trying to understand what the current code does and how to see it in action.

Yes! View the format settings after setting format to "table". There should be an added column of radio buttons with the heading "row header" in the table that has the field config.

Uploading screen shot of new interface...

Thanks @miwayha - this is very exciting progress! I'll review it more closely soon.

The new radio buttons for row header follows the same pattern as the default-sort configuration.

It has a "none" option which is great - it means a site builder can change their mind and go back to a single-header table if they wish. (In fact I did so several times, just to get some screenshots...)

The markup is what we expect. I modified the core /admin/content view so that the node "Title" is treated as a row header. Here's a screenshot of the markup:

The cell for the node title is a th element with a scope="row" attribute.

Updating issue summary to reflect the new plan as a feature request:

• Outline the plan for optional row headers, re-using bits of comment #8
• Storing the new row header option counts as a data model change.
• Adding @gettyburger's original report.

In the Seven theme, using the patch from #12, row headers inherit some different styling similar to the column headers.

• The text of a row header is bold (I like this).
• The row headers get the same hover effect as column headers, with a prominent underline (I think this looks weird).

For these screenshots I modified the core admin/content listing to output the node title as a row header. The mouse pointer is not incliuded in the screenshots, but they show the hover style on the second row.

Before patch #12, just column header:

After patch #12, treating node title as a row header:

We'll need design review for this, tagging for Seven maintainer too.

For completeness, here is how row headers look in Bartik.

Like the previous screenshot for Seven theme, this shows the admin/content listing, modified so node title is a row header. Then I set Bartik as the admin theme.

I like this, it's very clear, and matches the column headers perfectly.

Nonetheless, it needs design review sign-off. Tagging for Bartik maintainer.

Note: these screenshots also demonstrate that the proposed solution is flexible enough to support tables with an offset column of header cells.

I am very excited about this new feature!

We still need a code review of the patch from #12

I'm wondering whether we should move "row header" further to the right on the views admin screen. It seems to be for me that "sortable" might be more often changed for example. IMHO we need some better way to communicate what this feature does to the user.

we need some better way to communicate what this feature does to the user.

We could take the row-header options out of the table, and present them as a drop-down select, the way the grouping option is shown. Then it could have a #description of its own, which the options in the table don't have.

That might be indeed a good idea. Let's see whether someone from the UX team can help us with that.

My sandbox module for D7 uses a dropdown select, if you'd like to test with that rather than coding something from scratch.

Here's a screenshot from @miwayha's D7 sandbox module.

It shows a drop-down select for choosing the row header field, as described in #27.

If we choose this rather than radio buttons in the table, we'll need to decide what weight to give it in relation to the other settings.

For the usability review, we're comparing two ideas for the setting widget:

• a column of radio buttons (screenshot in comment 16)
• a drop-down select with a description (screenshot in comment #30)

For the design review, we're looking at the screenshot in comments #21-22.

Updating tasks

A select list on a separate config modal is less clear than the column of radio buttons on the table itself. But the config modal gives us a little more room to explain the feature. Could we maybe make the "None" option explain what this is all about?

I think I prefer the radio buttons on the table. But: it's another column. Could we add a setting on the config modal to enable multiple header selection in the first place?

I think Bartik looks fine. I'm not the maintainer, but I think this is what it should be.
For Seven we should apply the same styling as is used along the top.

I'm looking into this at the DrupalCon Dublin 2016

Excited to see what comes of this in Dublin. I'll keep an eye on this issue, and I'll be happy to contribute any other patches to it if need be.

Thanks so much for doing a ton of work on this, @andrewmacpherson and others!

I talked to yoroy and moving the column more to the right should be enough for now.

In the future it would be nice to be able to hide elements in the form because it gets a little full in the table.

I'll add a interdiff in a moment.

whoops made the patch from the wrong directory.
Here is a new patch with the core path included.

Just reworking a bit an if/else block so its easier to follow

Not sure what to review, yoroy already looked at this.

hi
it would have been nice if somebody could help us please with a patch for irregular headers in D7..

thanks

Patch not applying anymore, rerolling

Had to manually fix these two files:

• core/modules/views_ui/views_ui.theme.inc
• core/modules/views/src/Plugin/views/style/Table.php

Conflicts were due to the switch to short array syntax in core, so rerolled appropriately.

for my project , it is great full.

Just a patch cleanup:

--- a/core/modules/views/src/Plugin/views/PluginBase.php
+++ b/core/modules/views/src/Plugin/views/PluginBase.php

@@ -192,26 +192,6 @@ public function filterByDefinedOptions(array &$storage) {
-   * Do the work to filter out stored options depending on the defined options.
-   *
-   * @param array $storage
-   *   The stored options.
-   * @param array $options
-   *   The defined options.
-   */
-  protected function doFilterByDefinedOptions(array &$storage, array $options) {
-    foreach ($storage as $key => $sub_storage) {
-      if (!isset($options[$key])) {
-        unset($storage[$key]);
-      }
-
-      if (isset($options[$key]['contains'])) {
-        $this->doFilterByDefinedOptions($storage[$key], $options[$key]['contains']);
-      }
-    }
-  }
-
-  /**
    * {@inheritdoc}
    */
   public function unpackOptions(&$storage, $options, $definition = NULL, $all = TRUE, $check = TRUE) {
@@ -258,6 +238,26 @@ public function destroy() {

@@ -258,6 +238,26 @@ public function destroy() {
   }
 
   /**
+   * Do the work to filter out stored options depending on the defined options.
+   *
+   * @param array $storage
+   *   The stored options.
+   * @param array $options
+   *   The defined options.
+   */
+  protected function doFilterByDefinedOptions(array &$storage, array $options) {
+    foreach ($storage as $key => $sub_storage) {
+      if (!isset($options[$key])) {
+        unset($storage[$key]);
+      }
+
+      if (isset($options[$key]['contains'])) {
+        $this->doFilterByDefinedOptions($storage[$key], $options[$key]['contains']);
+      }
+    }
+  }
+
+  /**

No reason to move this method around that I know of, so undoing this from this patch to make it easier to review.

How to integrate with multi level header rows in this view table.

re: #54 - that's out of scope for this issue, but it's already mentioned in the issue summary as an idea for future work.

Thanks for mentioning the #893530: Multi level (multirow) header for table FAPI element here, though. I've linked to it in the issue summary here.

For views to produce multi-level headers, I think we'll have to wait until #893530: Multi level (multirow) header for table FAPI element is done first. Then we can open up a new issue to look at ways of letting a site-builder produce them with Views. Maybe we can extend the existing table views display in core, or it might need a separate views display plugin.

The patch as re-rolled applies cleanly. The formatting appears to check out as well.

In this image you can see the new table setting appears when editing a table view. This is a clone of the watchdog view.

Then, the

Sorry just drive-by Twig standards stuff for now.

1. +++ b/core/themes/classy/templates/views/views-view-table.html.twig
   @@ -106,7 +107,11 @@
   +          {% if (key) == (row_header) %}
   
   @@ -118,7 +123,11 @@
   +          {% if (key) == (row_header) %}
   

   Are the parens needed?

2. +++ b/core/themes/classy/templates/views/views-view-table.html.twig
   @@ -106,7 +107,11 @@
   +            <th scope="row" {{ column.attributes.addClass(column_classes) }}>
   ...
   +            <td {{ column.attributes.addClass(column_classes) }}>
   

   In both these cases, there should be no space before we print the attributes.

Thanks for the quick look @cottser.

It seems like the parenthes are not needed. It seems to work properly at least without them.

The spaces are removed prior to the attributes being printed.

Interdiff also attached.

@theMusician - thanks for working on this.

Review of patch #58:

1. The parentheses were removed from the template in Classy, but they're still present in the template in Views module.

2. --- a/core/modules/views/templates/views-view-table.html.twig
   +++ b/core/modules/views/templates/views-view-table.html.twig
   @@ -71,7 +72,7 @@
                  ]
                %}
              {% endif %}
   -          <th{{ column.attributes.addClass(column_classes).setAttribute('scope', 'col') }}>
   +          <th {{ column.attributes.addClass(column_classes).setAttribute('scope', 'col') }}>
   

   Unnecessary whitespace changes. I tested with Stark, and it has no effect on the output. I checked our Twig coding standards about this - there's no explicit opinion, but the examples don't have whitespace between element name and curly brace.

3. --- a/core/modules/views/templates/views-view-table.html.twig
   +++ b/core/modules/views/templates/views-view-table.html.twig
   @@ -94,7 +95,7 @@
      {% endif %}
      <tbody>
        {% for row in rows %}
   -      <tr{{ row.attributes }}>
   +      <tr {{ row.attributes }}>
   

   Same as previous point.

We'll need functional tests to check the output of the new option.

@cottser - Where do we stand with Stable and Classy themes here? We're adding a new UI option which won't work unless they have the right template. The current patch changes templates in Views module and Classy theme, but what about sites using base theme: false and have the Stable template?

I think there's low risk to existing websites, because they won't have row-level headers configured for table views. Until a site-builder makes use of the new option, the output will be unaffected.

TODO:

• Do we need to update existing views which use the table display plugin, to include the new config option (set to none)?
• A change record with advice for themers.
  • If they already have a custom views.view-table.html.twig template, it will need updated to benefit from this feature.
  • Turning the option on may have an unexpected effect on how tables look, if their theme doesn't already anticipate row-level table headers.

I reviewed the twig coding standards as well and agree, nothing is explicit but the samples do not have a space.

Attached is a patch that includes the update to the standard view twig template as well.

Regarding tests, as views has a lot of them. Where would one start?

It seems that any test file that includes an option for "sortable" would need to have "row_header" added as well.

From there, what other test files need to either be edited or created?

This doc helps for a simple module, https://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial, but the amount of tests in views has me a little perplexed.

Can we get a 58-63 interdiff?

This one is really close, tests are the last part needed.

@theMusician - this is probably going to be functional tests rather than unit tests. Look at tests inside core/modules/views/tests/src/Functional/ to find some. They typically make use of a test module inside tests/src/modules, and the test modules will have installation config like any other module. In views case, the views_test_config module has lots of specimen views.

Maybe we can add an extra test method to core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php. The test process will be something like:

• set up a test view which is configured with row-level headers
• get the HTML output
• assert the expected <th scope="row"> is there for the chosen column,
• assert it's just a <tr> for the other columns,
• assert there's only one column which uses row-level headers
• ... etc.

Also assert a table display with row-level headers configured as "none" does not have an TH inside the table body.

Thanks @andrewmacpherson for the guidance on the tests.

The interdiff is attached. Interdiff 58-63

If anyone is sprinting at DrupalCon and wants to tackle the tests please dive in!

Since we adding a new option we should really have an upgrade path for this to add the option to existing views. One problem with adding new settings to the table style is that existing settings are probably not up to date anyway, see #2917814: Views table style info never gets updated when changing/adding fields

Nitpicks:

1. +++ b/core/modules/views/src/Plugin/views/style/Table.php
   @@ -326,6 +332,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +        // Because 'radio' doesn't fully support '#id' =(
   

   This could be a more complete sentence explaining what's going on and why, and is there an open issue for that?
   "We need to set the id using attributes because 'radio' doesn't fully support #id."

2. +++ b/core/modules/views/src/Plugin/views/style/Table.php
   @@ -396,6 +419,17 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +    // Provide a radio for no row header
   

   Missing a trailing period.

3. +++ b/core/modules/views_ui/views_ui.theme.inc
   @@ -462,13 +467,20 @@ function template_preprocess_views_ui_style_plugin_table(&$variables) {
   +    array('data' => t('None'), 'colspan' => 2),
   +    array('colspan' => 4),
   +    array('align' => 'center', 'data' => $form['default'][-1]),
   +    array('align' => 'center', 'data' => $form['row_header'][-1]),
   +    array('colspan' => 2),
   

   old skool array() notation needs to be []

4. +++ b/core/themes/classy/templates/views/views-view-table.html.twig
   @@ -26,6 +26,7 @@
   + * - row_header: The field that should be marked up as a table header on each row.
   

   > 80 characters

5. +++ b/core/modules/views/templates/views-view-table.html.twig
   @@ -26,6 +26,7 @@
   + * - row_header: The field that should be marked up as a table header on each row.
   

   > 80 characters

@lendude

Thanks for the related issue. DO you think it'll be a blocker for this one? I mentioned the upgrade path in #62, but didn't update the issue summary. I'll do that now.

I've reviewed the comments going all the way back, and updated the tasks in the issue summary to says what's been done.

One thing that isn't quite clear: are we happy about how row-level headers look in Seven? There's a screensnot in #21.

Later in #33 @yoroy says "For Seven we should apply the same styling as is used along the top", but I don't think we've acted on this. I think it means the row-level headers should get the beige background colour. I also think the link underlines look odd, in the screenshot example which made the node title links as the row headers.

Let's seek a design review again for Seven.

Let's get some screenshots set up using the latest patch, and add them to the issue summary.

Here's a good novice task for the Nashville sprints...

1. Screenshot of the views table display settings. We want to show where the new setting is in the Views UI.
2. Update the "Content" view which powers the admin/content listing, make the node title field act as a row-level table header.
3. Then get a screenshot of the view, at admin/content. Make sure the mouse pointer is hovering over a node title, we want to confirm if the broad link underline from screenshot #21 is still an issue.
4. Switch the admin them to Bartik, and get a screenshot of admin/content, mouse over a node title.
5. Update the issue summary, put screenshots in proposed resolution.

Bonus level: find a member of the usability or design team, or a product manager, and get an opinion how it looks in Seven to clarify comment #33.

I'll work on getting those screenshots (here at Drupalcon Nashville)

DO you think it'll be a blocker for this one?

@andrewmacpherson probably not since we'd only add a setting that is not tied to the fields that exist on the view (well the default setting isn't anyway...), so I think we can workaround this.

Addressed my own nits.

Added the needed config schema changes and the most minimal of tests. If anybody wants to turn this into a proper test, go for it!

Added some screenshots to the proposed resolution section of the issue summary. I verified that the styling pointed out in #21 still applies for Seven and Bartik.

Regarding the bonus level task in #70, I spoke with Benji Fisher and dug into the CSS a little bit. It looks like the <th> tag background styles are specific to the <thead> section in the Seven theme, but the link underline style is not that specific. I changed the CSS selector in my browser inspector so that the gray background was applied to all <th> elements but Benji and I agreed that this looked a little strange. See the following screenshot.

With Hover:

By making the styles more generic, it includes the following CSS on all <th> elements:

th {
    background: #f5f5f2;
    border: solid #bfbfba;
    border-width: 1px 0;
    color: #333;
    text-transform: uppercase;
}

We also wondered whether that change might be out of scope for this issue since it would require a css change to the Seven theme.

Something I noticed wile writing tests for this, when the view has no results the empty result area gets turned into a th while the setting is set to 'none'. That doesn't sound like the desired behaviour.

@Lendude, thanks for outlining what the tests would look like. For anyone wanting to wrap up tests take a look at drupal/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php starting on line 250 once the most recent patch has been applied.

In regards to no results returning from a table styled view, I cannot recreate that occurrence.

It seems a no results default for views in table format is to simply not display anything at all. I cannot get an empty table to render if there are results.

@theMusician it was the 'empty message' that got set to a <th>, but I can't reproduce it anymore, no idea how I got it that state, but thanks for checking!

Adding tag for the sprint

@rakesh.gectcr - I'm attending the NWDUG sprint too. The remaining work here is to finsih the tests, and it would be good to look at the core theme styles more closely.

The novice tasks in #70 were done, not currently needed.

Gathering some issues up into a bigger-picture plan for improving tables.

Rerolled the patch from #72.

Trying a new editor and apparently my formatter is not firing on save. Another attempt.

Ok. I think I finally have PHP code sniffer configured properly with Lando.
Attempt at the re-roll #3.

And one more fix with functions needing comments.

Testing out the patch in #90, I could get the template to print correctly in the view, with a <th scope="row">, if I didn't include the fixes with the views_ui.theme.inc.

Adding in the changes from the views_ui.theme.inc file caused the row header settings to no longer apply correctly. But, without adding the views_ui.theme.inc changes, the table settings is no longer usable:

Wondering if anyone else is running into this applying the patch from #90, or if it's just something I'm doing.

Patch includes Olivero and Bartik view table templates, with row scope support. This still uses tests written by @lendude referenced in #75.

Cleaning up PHP files through phpcbf based on previous test results.

Re-rolled #95 for 9.4.

Including Umami views-view-table twig file to fix classy copy error. (Duplicate patch error--previewing comments made it seem the file was removed, had re-added just in case.)

Applied patch #96 in drupal-9.3.x-dev applied successfully
After patch row header has been added
Thanks for the patch
for ref sharing screenshot ...

This patch updates the hash for the file views-view-table.html.twig which appears to have been causing the failed tests. This works locally running the test ConfirmClassyCopiesTest.php --filter=testClassyHashes with phpUnit.

Updates classy view table templates in Claro and Seven to fix test results in #100.

The patch in #101 works for 9.4.x and prior recent rerolls work for earlier Drupal installs.

The issue summary has been updated to reflect where I believe this issue now stands.

In the remaining tasks, there exists

In reviewing that comment I am unsure if this is truly a blocker for improving the accessibility options available in Drupal views.

Change record coming for review.

Please review the change record at https://www.drupal.org/node/3263530.

Primarily this was based on the prior recommendations and the work discovered while crafting the patches with everyone on this issue.

Interdiff between failing 9.3.x #95 and passing #101 for 9.4.x. Patch couldn't apply to 9.3.x because of conflicting comment.

Refactored #104 patch to cleanly apply to 9.3.x.

Reroll #106 to add stable and starter-kit view templates.

Reroll #101 to add stable and starter-kit view templates for 9.4.x.

Reroll #107 to pass php formatting tests in 9.3.x.

#109 is working nicely for me.

Please don't add unrelated coding standards changes to these patches. It makes them bigger and so much harder to review because we now need to think about if a change is needed.

Setting back to needs work to limit this to the changes that are actually needed for this feature.

Thank you for the feedback.

Can you help me understand what is unrelated? In 9.4.x it seems like there are a lot of views-view-table twig files in Drupal core. Without including those files, the tests fail to pass I believe due to /Core/Theme/ConfirmClassyCopiesTest.php. As I understand it, if Bartik for example has a copy of the views-view-table twig file and it is not updated, the accessibility improvement in this patch would be ignored because that would override the template default.

Beyond updating those files and the hash to make the tests pass 9.x I am not sure what else is not related.

If those files do not need to be updated, what is the trick for making the tests ignore them?

More than happy to do the work, I just don't know what I don't know.

@Lendude, Here I tried to remove all CS corrections that weren't necessary for the patch.

Patch # 115 tested and applied successfully in drupal 9.5.x version.

I think we need to target 10.1.x for this feature request as we're in beta for 9.5.x so it's closed to new features. Plus the patch doesn't apply to 10.1.x so we need to 10.x version anyway.

+++ b/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php
@@ -242,6 +242,23 @@ public function testGrouping() {
+    $this->assertNotEmpty($this->cssSelect('tbody th'));

Can't we assert on the actual content. Or something a bit more positive. Also shouldn't we be selecting on scope="row"

Plus we have no test coverage that the UI actually works.

This doesn't get us a UI test coverage fix but I believe this is now applying cleanly to 10.1.x-dev.

The original patch from 9.5.x trying to apply to 10.1.x-dev.

git apply --check ../2770835-115.patch
error: core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php: No such file or directory
error: core/themes/bartik/templates/classy/views/views-view-table.html.twig: No such file or directory
error: core/themes/classy/templates/views/views-view-table.html.twig: No such file or directory
error: core/themes/seven/templates/classy/views/views-view-table.html.twig: No such file or directory
error: core/themes/stable/templates/views/views-view-table.html.twig: No such file or directory

This makes sense as those files have been removed from 10.1.

Attached is a new patch and a diff of the reroll showing the files being removed from the new patch.

This fits under WCAG SC 1.1.1.

Fixed failed commands on #120
Attached patch against Drupal 10.1.x

Still needs test coverage for the Views UI working.

1. +++ b/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php
   @@ -242,6 +242,24 @@ public function testGrouping() {
   +    $this->assertNotEmpty($this->cssSelect('tbody th[scope="row"]'));
   

   This is still too minimal, we should actually check what is in it

2. +++ b/core/themes/olivero/templates/views/views-view-table.html.twig
   index a26eaeba85..a212ef8dcc 100644
   --- a/core/themes/stable9/templates/views/views-view-table.html.twig
   
   --- a/core/themes/stable9/templates/views/views-view-table.html.twig
   +++ b/core/themes/stable9/templates/views/views-view-table.html.twig
   

   I don't think we can change these here templates since they are 'stable'

And looking at this again, we should add an upgrade path to set the default value for existing Views so people don't get unrelated changes later when they change a View

Thank you for the feedback Lendude. Items 1 and 2 I believe I have knowledge of how to address moving forward.

Regarding an upgrade path for existing views, in the proposed patch this is set in Views/Style/Table.php,

$options['row_header'] = ['default' => ' '];

For existing views, where would I look to provide an upgrade path? In local testing, existing views seem to show the new row header option but not have it selected. I am guessing I am missing something obvious.

I appreciate any guidance you can offer.

Thank you

A new patch that does a more robust check for scope=row on output.

Removal of changes to stable9.

Thank you @CarlyGerard and @packern for team programming on this patch.

Missed the code formatting fixes. Trying again.

I believe the new patch satisfies the concerns raised earlier in the discussion.

Cleaned up the tags some.

Seems #124 still needs to be addressed.

For the upgrade path it will, I believe,
1. need to load all views
2. Check if they are using table
3. Add this setting even if it's unchecked

Idea being if this patch is applied and I open a view with a table. If I make 0 changes and click save and configuration is shown as changed because of this new setting then that would need upgrade path.

Upgrade test can be simple
Check a view table that this setting doesn't exist
Run updates
Check setting now exists.

Adds upgrade path test to views where if not set, set header to "none." The interdiff showed a template also removed a classy template--I think in 11.x-dev the classy files are removed, unless someone can confirm otherwise?

Re-rolled patch from comment #131 since it couldn't apply.

Fixing custom commands error from ViewsConfigUpdater.php in #132, where the bool value wasn't returned properly given the function parameter.

Fixes undefined array key issue flagged from ViewsConfigUpdater.php in last CI testing.

Hooray. Passing tests again and they are more comprehensive. I believe this checks all the tests described in comment #129.

The change record has been updated to reflect the current passing patch.
https://www.drupal.org/node/3263530

+++ b/core/modules/views/views.post_update.php
@@ -137,3 +137,14 @@ function views_post_update_taxonomy_filter_user_context(?array &$sandbox = NULL)
+    return $view_config_updater->setRowHeaderOption($view);

This should follow the pattern of 'needsABC' and further changes to ConfigEntityUpdater so it will also update newly imported config when installing a module like we do in other Views updates.

Also we need a test for the upgrade path to make sure it actually works

As always, thank you for the feedback and guidance.

On my dev instance, there is an upgrade test in theory that operates but I can't get views actually to run an update hook.

A new file in tests/fixtures/update exists called set_row_header_option.php

It looks like

/**
 * @file
 * Test fixture.
 */

use Drupal\Core\Database\Database;
use Drupal\Core\Serialization\Yaml;

$connection = Database::getConnection();

$connection->insert('config')
  ->fields([
    'collection' => '',
    'name' => 'views.view.test_set_row_header_option',
    'data' => serialize(Yaml::decode(file_get_contents('core/modules/views/tests/fixtures/update/views.view.test_set_row_header_option.yml'))),
  ])
  ->execute();

and then in tests/src/Functional/Update/ViewsSetRowHeaderUpdateTest.php I try to run the test which includes firing the updates using runUpdates(); which as I understand it runs update hooks. It is modeled after ViewsFixRevisionUpdateTest.php in the same directory.

    /**
   * Tests the upgrade path for adding row header option.
   */
  public function testViewsPostUpdateSetRowHeaderOption() {
    $view = View::load('test_set_row_header_option');
    $data = $view->toArray();

    // Assert no row header exists
    $this->assertArrayNotHasKey('row_header', $data['display']['default']['display_options']['style']['options']);

    $this->runUpdates();

    $view = View::load('test_set_row_header_option');
    $data = $view->toArray();

    // Assert row header option exists, return values
    $this->assertArrayHasKey('row_header', $data['display']['default']['display_options']['style']['options']);
  }

which I think means the update hook needs to be in views.install. I have tried various iterations but when runUpdates() executes it doesn't apply any changes. Is there a different spot that updates go to when running tests? I can't find any views updates that would correspond to the other tests in /Tests/Src/Functional/Update.

I don't get any errors in the tests except that after running the updates, the new key cannot be found which tells me the hook_update I added into views.install is not running properly. If I introduce a syntax error into the hook_update, the test will fail and point out the syntax error, so it does seem like it wants the code to be in that spot.

Thank you for any hints.

I believe the code for ViewsConfigUpdater needs to be called https://git.drupalcode.org/project/drupal/-/blob/96d21a4603df23080a34051... in updateAll of viewsConfigUpdater for the test runUpdates() function to work.