🇺🇸United States @theMusician

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.

Thanks for the feedback. I agree that the undefined array key error is because the views did not get the update to set grouping_label_element to null.

The ViewsConfigUpdater code requirement makes sense to me as being necessary. On a separate issue, https://www.drupal.org/node/2770835 → , the test update never fires even though I wrote a function in ViewsConfigUpdater but I was not aware you have to also call the function in updateAll(). The documentation improvements for how to use ViewConfigUpdater is a great idea.

Here is a rough idea of what I think is needed. @CarlyGerard also involved with this ticket walked me through what this may end up requiring. In the protected function addGroupingLabel() I can't quite figure out what the handler represents. The parameter defines it as a display handler, so I think a view style is represented here as the handler_type "style" and that is also the plugin_id? These ideas came from one of the issues you referenced, thank you for the pointer - https://git.drupalcode.org/project/drupal/-/blob/96d21a4603df23080a34051....

  /**
   * Performs all required updates.
   *
   * @param \Drupal\views\ViewEntityInterface $view
   *   The View to update.
   *
   * @return bool
   *   Whether the view was updated.
   */
  public function updateAll(ViewEntityInterface $view) {
    return $this->processDisplayHandlers($view, FALSE, function (&$handler, $handler_type, $key, $display_id) {
      $changed = FALSE;

      if ($this->addGroupingLabelElement($handler, $handler_type, $view)) {
        $changed = TRUE;
      }

      return $changed;
    });
  }

  /**
   * Add Grouping Label to views without one.
   *
   * @param array $handler
   *   A display handler.
   * @param string $handler_type
   *   The handler type.
   * @param \Drupal\views\ViewEntityInterface $view
   *   The View being updated.
   *
   * @return bool
   *   Whether the handler was updated.
   */
  protected function addGroupingLabelElement(array &$handler, string $handler_type, ViewEntityInterface $view): bool {
    $changed = FALSE;

    // Add grouping label element to existing views.
    if (($handler_type === 'style')
      && isset($handler['plugin_id'], $handler['type'])
      && $handler['plugin_id'] === 'style'
      && $handler['type'] === 'Grid' || 'HtmlList' || 'GridResponsive' || 'DefaultStyle'
      && !isset($handler['style']['grouping_label_element'])) {
      $handler['style']= ['grouping_label_element' => NULL];
      $changed = TRUE;
    }

    return $changed;
  }

You asked, why the allowed grouping elements would not be restricted to just heading tags like the views pager update. For the grouping use cases, grid, HtmlList, GridResponsive it seemed hard to predict that one would only ever group by heading. Using labelELements, https://git.drupalcode.org/project/drupal/-/merge_requests/7351/diffs#11..., makes it so available elements do not need to be maintained in this code as well as the views.settings. I concur that the most common use case would be headings but I am not confident that there would never be a need to group by another element.

Thanks again for the feedback, the questions, and the guidance.

The test had been failing due to this upstream bug, that has now been fixed.

https://www.drupal.org/project/drupal/issues/3448036 🐛 InstallerTranslationExistingFileTest fails on 11.x branch Active

Tests are now passing.

Good morning,

I have authored a proposed change record for review by others. https://www.drupal.org/node/3446094 →

I have also pushed an updated branch that includes theme updates to the core themes where the views templates are included. I did not, however, update Stable 9 as my understanding is that the Stable 9 theme is locked in place. I am happy to make the changes there if required.

Thank you for the feedback.

To answer your question about if a view does not have $usesGroupingLabelElement present, the twig templates handle that logic and print the title. https://git.drupalcode.org/project/drupal/-/merge_requests/7351/diffs?co...

If it is present, such as after this patch is applied, but not set, the code supports the current default of the grouping label being an h3 or empty.
https://git.drupalcode.org/project/drupal/-/merge_requests/7351/diffs?co...

Contrib plugins would have an option of using this way of handling additional HTML options or could continue doing it however they currently make it happen.

There are a few of the core themes that use the twig template overrides for views-view-unformatted.html.twig, views-view-list.html.twig, and a few more. You suggest updating these in this same patch. As I understand it, the twig templates would be updated where appropriate for claro, olivero, stark, and starterkit_theme but not stable9. The same approach would be for engines/twig excluding updates to stable9. Is that correct?

You are right, we don't have any other annotations. I thought there was a section of code but it turns out I was wrong.

As we get a 3.1.x release set we'll work this in.

Setting to needs review.

The attribute conversion looks good. We'll keep this branch open as more attributes become available. As we prepare to move to support 10.x and greater versions of Drupal and remove support for 9.x, we can spin a new release that has the attributes in it and leave a 3.x version available for all on Drupal 9.x.

Thank you for your contributions to this work.

New code appears to be passing.

theMusician → made their first commit to this issue’s fork.

Thanks Vivek. Not sure we need to target against 10.3.x, this should all work against the 11.x branch. The tests themselves don't run currently, so seeing them fail on the pipeline is not surprising. The project has the goal of getting tests working, but any of the functional tests have stymied us.

Closing as now drupal rector is working against the 11.x branch and posting into the queue.

The new 2.0.5 release fixed this issue where I was seeing it.

It appears the patch was applied cleanly. There is what seems to be an unrelated error in a unit test when testing the merge request.

https://git.drupalcode.org/issue/drupal-1383696/-/jobs/1255878

Drupal\Tests\editor\Functional\EditorSecurityTest 0 passes 1 exceptions
FATAL Drupal\Tests\editor\Functional\EditorSecurityTest: test runner returned a non-zero error code (2).
Drupal\Tests\editor\Functional\EditorSecurityTest 0 passes 1 fails

---- Drupal\Tests\editor\Functional\EditorSecurityTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Other      phpunit-93.xml       0 Drupal\Tests\editor\Functional\Edit
    PHPUnit Test failed to complete; Error: PHPUnit 9.6.15 by Sebastian
    Bergmann and contributors.
    
    Testing Drupal\Tests\editor\Functional\EditorSecurityTest
    ..E                                                                 3 / 3
    (100%)
    
    Time: 00:23.789, Memory: 4.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\editor\Functional\EditorSecurityTest::testEditorXssFilterOverride
    Behat\Mink\Exception\ExpectationException: The field "edit-body-0-value"
    value is "Hello, Dumbo Octopus!alert(0)", but "Hello, Dumbo
    Octopus!alert(0)" expected.
    
    /builds/issue/drupal-1383696/vendor/behat/mink/src/WebAssert.php:888
    /builds/issue/drupal-1383696/vendor/behat/mink/src/WebAssert.php:781
    /builds/issue/drupal-1383696/core/tests/Drupal/Tests/WebAssert.php:968
    /builds/issue/drupal-1383696/core/modules/editor/tests/src/Functional/EditorSecurityTest.php:447
    /builds/issue/drupal-1383696/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    ERRORS!
    Tests: 3, Assertions: 137, Errors: 1.

theMusician → changed the visibility of the branch 1383696-how-to-change to hidden.

The automatic addition of the project name to the job is smart.

That will smooth things out for a lot of projects I bet. Congratulations on simplifying the quote pattern.

Thanks for your help originally with this as well.

@jonathan1055 reviewing the latest pipeline build the changes look good. CSPELL passes.

Thanks for the feedback. There should not be any additional steps to make the interactive transcript work for local videos. This is a screenshot from a local video I use to test.

I'll review the README and try to improve the documentation. Thank you.

Committed to 3.x

Forgot to use the create issue fork for this fix.
https://git.drupalcode.org/project/ableplayer/-/jobs/1077005

Thank you. The tests need work and every little bit helps.

theMusician → created an issue.

The gitlab process is now running. There is work to do on the cspell job and phpcs and phpstan in our /tests directory. We will open additional issues for these.

The updated instructions were helpful. I can now get cspell to ignore paths.

I think this example in the md file needs the final single and double quotes reversed.

Instead of

variables:
  _CSPELL_IGNORE_PATHS: '"tests/data/*", "**/*.log<strong>'"</strong>

It would be

variables:
  _CSPELL_IGNORE_PATHS: '"tests/data/*", "**/*.log<strong>"'</strong>

That syntax is a lot nicer than the escaping and double quotes in the original help text. Thanks for improving it!

My cspell job still exits with an error code of 1, but is now properly ignoring the paths and skipping over words that I ask it to, so solid progress. Gitlab CI claims I still need a configuration file,
Configuration Error: Failed to read config file: "/builds/project/ableplayer/gitlabci_cspell/ableplayer/.cspell.json". The configuration output file looks correct so I must still have something just a little off.

Thanks again.

Thanks for the patch. With Drupal's move to gitlab CI I think the fork process has to now be followed to pull in your patch if you want to see it run the tests.

If not, I can look to move this into the code.

Thanks again.

This is a nice addition. I was trying to implement the ignore paths on a contrib project and I can't get it to work. Thank you @mparker17 as well for the how to exclude with double quotes. Without it, YAML complains about syntax.

https://git.drupalcode.org/project/ableplayer/-/blob/bronsem/3402263/.gi...

However, CSPELL still reviews the excluded directories.

Is the ignore paths just working for core at this moment?

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.

The 3.x branch is now getting the Drupalbot attention

Thank you so much for finding that and sharing the fix.

theMusician → made their first commit to this issue’s fork.

Latest bot patch merged in.

theMusician → made their first commit to this issue’s fork.

Ok. Giving this a go. A pipeline is running. The functional unit tests failed, which I am aware of before this. Curious to see how all of the lint testing goes.

https://git.drupalcode.org/project/ableplayer/-/pipelines/58102

Thank you for the work on this issue.

Right now the annotations are only supported for the following per the change record.

Annotation class Attribute class Available since
\Drupal\Core\Annotation\Action \Drupal\Core\Action\Attribute\Action 10.2.0
\Drupal\Core\Block\Annotation\Block \Drupal\Core\Block\Attribute\Block 10.2.0

Right now Able Player uses neither of these plugins. Will continue to monitor.

Thank you for the additional information. I have seen inconsistent behavior on sites built prior to 8 including media. Your note about the install profile makes me think you discovered what was causing that.

Most of the config is stored in the optional directory so maybe since it is looking at a prior installed site the config is not being pulled in? It should on the first and subsequent installs of the module but it seems like it is being skipped from your digging.

Able player media caption is distinct in case sites are doing translation, it lets you load the caption files in many languages.

I think the patch is a good path forward, especially if this is an edge case, if it is. With your patch, after install does everything work properly once you create the form?

Thank you. That works super. I appreciate you finding and offering the fix.

@geofferyr can you outline in more detail how you found and replicated this error? I did a fresh install, and deleted the local and video media types. I deleted their entity form configs via drush and the installs still worked with your code, but no error message was ever logged. I tried it with just one media type deleted and then deleting the config and it was reinstalled upon enabling able player.

Thank you

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

theMusician → created an issue.

Oh yeah, looks like that changed between D7 and 8 and we missed it.

https://api.drupal.org/api/drupal/core%21modules%21help%21help.api.php/f...

Thanks for bringing this up. I'll try to test soon. I typically do module work on Fridays which is a national holiday this week in the US so this might take a few weeks for me to review. If anyone else wants to try to test the change and post a screenshot and the URL they used to access the help page, that would be super.

Thanks again MukhtarM.

theMusician → created an issue.

Thank you. We'll review this soon. I appreciate the report and an idea of at least helping work around the issue while we figure out why.

I think the standard profile includes media but if another profile is used it would be possible for media not to be enabled. I have also seen sites that were not D9 originally, so those upgraded from prior versions where media types are not necessarily named the same because one had to create them manually in the past. This is especially true for local video types.

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

theMusician → created an issue.

The composer route did not pan out. Merging.

Adding the code commit MR as a patch file to play nice with a composer workflow. This will allow the module to maintain synchronicity with the Able Player parent project while allowing for patching of the parent project JS in between releases.

Thank you odelin. I'll try to get your patch in place tomorrow and update dev. Long-term I worry about patching the JS directly as when Able Player itself updates, we'll need to verify the patch is no longer needed or still needed and reroll.

I wonder if I can add the patch via composer with the module?

Thank you again for reporting this and supplying a fix.

Max

I tried to reproduce the issue and have not succeeded. Is this for a remote video or local video? Can you share thee exact settings you tried to save that did not take effect until refreshing the page?

Thanks

Thank you for the report and patch.
I believe this is related to the Able Player project JS itself, https://github.com/ableplayer/ableplayer/issues/581. I wonder if their proposed fix would also resolve it here until the core project releases a new update?

This is solved in 3.x.

theMusician → created an issue.

We believe we have a working solution.

The installed configuration was moved from the config install directory to the config optional directory. This keeps the media references within Drupal happy. The uninstall hook then was deemed not necessary so that has been removed to allow Drupal to manage dependencies.

We are going to test a few more variations of the module being installed fresh, existing, and uninstall/reinstall but so far this maintains the data integrity.

The branch issue-3367081 is current with the altered code.

Started working through this issue.
https://git.drupalcode.org/project/ableplayer/-/tree/issue-3367081

I tried this on a fresh install and on a local copy that I have had around for a few update cycles, beta 4, 5, 6 and I do see that the media references break on uninstall. They appear in Content -> Media but clicking on them throws an error.

Error: Call to a member function getSource() on null in Drupal\media\Entity\Media->getSource() (line 137 of /app/core/modules/media/src/Entity/Media.php).
Error: Call to a member function label() on null in Drupal\media\MediaForm->form() (line 25 of /app/core/modules/media/src/MediaForm.php).

However, Content -> Files still has the files, and clicking them shows their content.

Reinstalling the module, makes the media references work again.

I will have to research further how to keep the media references valid after the module is uninstalled. When that is resolved moving to a stable release seems prudent. If you have any ideas on how to keep the media references valid, I always appreciate contributions, ideas, code, and feedback like this.

Thank you.

That is great to hear you are considering it for a Drupal distribution. I have been looking at using it as a media player in Archipelago too.

We have been doing a lot of testing on sites we run hoping to get to a stable release.

I will need to test the install/uninstall and figure out a plan for making the transcripts stay in place or possibly give users the option to remove them.

Thank you for the feedback

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

Missed the code formatting fixes. Trying again.

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.

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

Would you allow the user to move the container or close it if they wish?

The Media Caption formatter is used by Able Player to transform the caption file output into the format Able Player itself expects. There is no real need to use it as on option for output as a selectable formatter. I can look into finding out if there is a way to hide it from the front end as I can see how it may cause confusion.

Alpha6 is out now with the D10 info file. Thanks for the support on the project jrglasgow.

This is fixed in the alpha6 version released.

Thank you for the tip. I'll try to clean that up later this week. I can see how the dev branch would be causing confusion. Certainly not my intent.

Max

It is merged into 3.x-dev. https://git.drupalcode.org/project/ableplayer/-/blob/3.x-dev/ableplayer....

I made some progress on this.

The audio plugin code has not been looked at for some time so there was some necessary cleanup.

It now knows about captions but does not show the output in the caption window. I am not sure why at this point in time. No errors are being thrown.

If someone is trying to recreate this on a fresh install, the code does not make Able Player the default audio player upon install. To do that go to admin/structure/media/manage/audio/display and select Able Player Audio as the formatter.

I also noticed, and this could be a 10.x quirk, that the Able Player Caption media type has the file field disabled upon a fresh install. To fix that, go to /admin/structure/media/manage/able_player_caption/form-display and drag or move via weight the file filed into the display section.

Clone the work in progress branch if you would like to try it out. It should work on 9.x core as well.
https://git.drupalcode.org/project/ableplayer/-/tree/bronsem/3340900

Thank you. It seems like Drupal is not supposed to aggregate external libraries, https://www.drupal.org/docs/develop/creating-modules/adding-assets-css-j... → , but it couldn't hurt to make it explicit as you do with the patch. Especially since that makes it work.

Thank you for the feedback. I believe the plugin that handles captions will either need to be extended for audio file output or updated.

Also on 3.x-dev are updated templates using apply spaceless and endapply in place of spaceless and endspaceless which are not valid in D10 twig.

file_get_url has also been replaced.

Thank you for the bug report. This has been fixed in 3.x-dev.

Thanks jrglasgow. The patch has been applied to 3.x-dev.

Starting at comment #54 ✨ Add aria-sort attribute to Views table output Fixed patches have started to be used. The existing MR 3222 claims it is mergeable but when one tries, it fails with a composer error.

That composer error seems related to a gitlab infrastructure snafu that occurred around the time the MR was updated. Not it seems stuck.

Hi @jrglasgow. Thanks for reporting this and supplying a patch file.

I can't reproduce the issue on my end. What version of Drupal are you using? Before merging the patch I want to be able to reproduce the issue. I agree it sounds just like the last time this occurred within the project.

Thank you.