🇺🇸United States @theMusician

Thanks for finding the issue. Your fix works on my end to get past the noted error. I think in the Azure AI portal I still need to work on allowing my dev box access to the API but it does now make the call.

I believe the ViewsUpdateConfig functions now are properly looping through existing views and setting the grouping to the default if it is not already set.

If using grouping on existing views, the default is the

element as it is already set to, after the config update executes.

themusician → changed the visibility of the branch drupal-1383696-drupal-1383696-82 to hidden.

themusician → changed the visibility of the branch drupal-1383696-drupal-1383696-82 to active.

Work internally continues on this.

The non-test files now pass PHP Stan.
https://git.drupalcode.org/issue/ableplayer-3502026/-/jobs/4520088

Thank you for the follow-up. I appreciate the extra information.

Thank you again @jonathan1055 for sticking with this one. I had not realized that the dictionaries CSPELL pulls from now are multiple, https://git.drupalcode.org/project/drupal/-/blob/11.1.2/core/.cspell.jso....

Merged and credit provided.

If we can help in the future via this project, please reach out.

The current MR works appropriately with Flash 2.0 as well.

If I don't run the modified code, I get the notices described in the issue summary.

Hi,

The simplified version is now in place. https://git.drupalcode.org/project/ableplayer/-/blob/3.x/.gitlab-ci.yml#L61

Thank you

I tried it again today before closing this and it seems the Azure Provider is again falling down. The general help message that appears states,

Warning message
We could not create an Alt Text, please try again later.

When attempting to generate alt text I get the original error in the logs:

Error: Call to a member function chat() on null in Drupal\ai_image_alt_text\Controller\GenerateAltText->generate() (line 135 of /app/web/modules/contrib/ai_image_alt_text/src/Controller/GenerateAltText.php).

Backtrace:

#0 [internal function]: Drupal\ai_image_alt_text\Controller\GenerateAltText->generate(Object(Drupal\file\Entity\File), 'en')
#1 /app/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)
#2 /app/web/core/lib/Drupal/Core/Render/Renderer.php(638): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#3 /app/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#4 /app/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
#5 /app/vendor/symfony/http-kernel/HttpKernel.php(181): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#6 /app/vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#7 /app/web/core/lib/Drupal/Core/StackMiddleware/Session.php(53): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#8 /app/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#9 /app/web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php(28): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#10 /app/web/core/modules/big_pipe/src/StackMiddleware/ContentLength.php(32): Drupal\Core\StackMiddleware\ContentLength->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#11 /app/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#12 /app/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#13 /app/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#14 /app/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#15 /app/web/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#16 /app/web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#17 /app/web/core/lib/Drupal/Core/DrupalKernel.php(741): Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#18 /app/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#19 {main}

Swapping to another provider and it works.

This appears to be fixed with the 1.0.3 release of AI.

After upgrading to AI 1.0.3 this is now working on a local install.

Hi,
Thanks for touching base. With the latest Drupal and the 2.3 version all seems well. Apologies for the false alarm. I couldn't get it to work on three different installs but now it works.

Cheers

themusician → created an issue.

Thank you. I can confirm using the stand-alone OpenAI module it works great. It does seem to be an issue with the Azure AI module.

I also get errors when trying to use the core AI module content suggestions, config/ai/suggestions.

This is what is returned when trying to analyze the page title for an improved title:

Error: Call to a member function setChatSystemRole() on null in Drupal\ai_content_suggestions\AiContentSuggestionsPluginBase->sendChat() (line 310 of /app/web/modules/contrib/ai/modules/ai_content_suggestions/src/AiContentSuggestionsPluginBase.php)

and this is from trying to analyze the body content for readability:
Error: Call to a member function setChatSystemRole() on null in Drupal\ai_content_suggestions\AiContentSuggestionsPluginBase->sendChat() (line 310 of /app/web/modules/contrib/ai/modules/ai_content_suggestions/src/AiContentSuggestionsPluginBase.php)

themusician → created an issue.

This branch, bronsem/code-cleanup, is now passing phpcs tests. https://git.drupalcode.org/issue/ableplayer-3454701/-/jobs/3976580

Open to further improvements if any exist.

Thank you.

Thank you Scott. We'll get this evaluated. I appreciate you trying to get the upstream project updated as well.

I have pushed a branch up that I have fixed all issues in src/, and phpcbf --standard=Drupal fixed in the translations directory. Translations are supplied by the Ableplayer project itself, so perhaps those should be excluded.

If one runs phpcs --standard=Drupal . within the Ableplayer directory there are just a handful of errors to resolve within the /tests directory.

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

Great catch. Sorry that slipped by for a few years now.

Hi,
The library is version 4.5.
https://git.drupalcode.org/project/ableplayer/-/blob/3.x/js/ableplayer.m...
Are you experiencing behavior indicating older JS is being used?

Thank you Mark. The new release has been authored.

Thank you Mark. I'll see what I can do this week. The changes look appropriate. Thank you for working through a manual test of the functionality as well. It seems to be all well and good on my local D11 too.

Are you using this at UT Austin?

themusician → created an issue.

Thank you for catching that and offering the patch. Kudos!

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

liberatr → credited themusician → .

Hi kylehuynh,

I have tried this as well on both a 10.2.x site using AblePlayer 3.1.0 and and a 10.3.x site using the dev branch and cannot recreate it with a local video file. Can you swap your theme on your site in a development environment? I wonder if some sort of CSS or JS in your theme is causing a conflict.

Thank you again for reporting the issue.

Thank you for reporting this. I can confirm the issue. It does not appear to be present in the core Able Player library demo, https://ableplayer.github.io/ableplayer/demos/video2.html.

I'll do my best to look into the possible causes.

the_g_bomb → credited theMusician → .

theMusician → created an issue.

Confirming this is still an issue in 2.0.5. Our use case is similar to trackleft2. We are able to work around this for now as we don't have many instances where we use multiple containers.

Oh, sorry to hear of that happening. I didn't realize dev was being used in production. I agree what you experienced is reproducible.

3.1.0 released, composer require 'drupal/ableplayer:^3.1'.

Max

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.