Add support for tables with two headers in views table display

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 ✨ Add support for tables with two headers in views table display Needs work since it couldn't apply.

Fixing custom commands error from ViewsConfigUpdater.php in #132 ✨ Add support for tables with two headers in views table display Needs work , 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.