CKEditor 5 loads all plugin translations on AJAX operations

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies.

@twod mind switching the branch

I’d recommend rebasing too

Left some comments and questions.

I just discovered this in #3232810: [PP-1] Allow setting aggregation groups for js files in library definitions and an issue search brought me here. For that issue, it would be ideal if we could remove the placeholder library/file altogether - I think the bug here shows it's conceptually wrong to rely on a 'global' placeholder when js can be loaded by the AJAX system and big pipe. Given the logic here works without the placeholder necessarily being there, that might be enough?

Just discussed this with @Wim Leers at DrupalCon.

He had the idea of a loads: false key for library definitions, we could then exclude any library with that set from the ajax page state already loaded libraries - and this would then allow all the logic in locale and ckeditor5 to work without any further changes.

I started work on the approach outlined in #12, but realised I wasn't convinced about adding anything to the libraries/asset API for this.

Then I had another idea about altering every relevant library to add a placeholder file or similar, and using that instead of the presence of the library, and that sort of worked.

Then I had another look at the original MR here, and realised the only thing I wasn't sure about was looking for the file sometimes and the library others - we can check for the library all the time in the two places it's likely to be.

Updated https://git.drupalcode.org/project/drupal/-/merge_requests/11678 for that, had to update the unit test for changes in HEAD since and a slight adjustment for the new logic, but this seems fine.

Might try the same on #3525743: Locale JavaScript translation doesn't take into account AJAX too.

Which MR should be reviewed?

Hid two of the MRs. Given that https://git.drupalcode.org/project/drupal/-/merge_requests/11678 is passing tests, I don't think we need to explore the 'add the fake file to every library' approach.

A couple comments about the test, one about mocking that I think needs fixing, the other a nit about whitespace.

I also tested this manually and confirmed that the translation JS files were downloaded per reproduction step 12 on 11.x while they were not on the MR branch.

Thanks. Applied both of the MR comments.

Lgtm.

Committed and pushed 266483960f2 to 11.x and 0d063d19c8d to 11.3.x. Thanks!

We might need to revert this.

In Umami, all 847 ckeditor translation files are being loaded on https://drupal-dev.ddev.site/en/node/add/page

I have a performance test from another issue that would have detected this, not sure why the logic doesn't actually work - seems like the library we're looking for is not actually included in $assets->getLibraries() by the time we get to hook_js_alter().

Reverted for now.

There's a quick fix - we need to get all the dependencies of the libraries for the assets, not only the libraries in the assets themselves.

https://git.drupalcode.org/issue/drupal-3516264/-/jobs/8116203#L479 is a new performance test that I added with the commit that was reverted.

That test passes again when we check against libraries with their dependencies.

This is back to green again with additional test coverage.

The problem was the original fix didn't check whether ckeditor5.translations was in the dependencies of the libraries from the assets passed in. This meant there were still some (different) cases where ckeditor5_js_alter() wouldn't filter out all of the translation libraries. Once we check dependencies every case should be covered.

I added a new method to AssetAggregationAcrossPagesTest borrowed from #3565258: Support library-specific aggregates (which is how I found out the fix was incomplete). Also had to update the unit test anyway and that now covers when the library is only in the dependencies and not explicitly passed in.

I've also opened #3568295: Only create ckeditor5 translation libraries for enabled languages because the symptom of this bug is that approximately 900 files get added to a page, even if the bug is fixed, I don't think we need to add all of those files and then remove them again, when it's only ever possible that we'd return a subset.

At some point it would be worth looking to see if there's a completely different way we could handle these translations without the placeholder file etc., but this at least should be an improvement.

Bumping to major because this is blocking generic improvements to the asset aggregation system that expose the bug in different ways.

@twod the change since the last RTBC/commit/revert is very small, would you feel comfortable re-RTBCing?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Bot rebellion

Committed c8c8cb5 and pushed to main. Thanks!
Committed 69e93b3 and pushed to 11.x. Thanks!

I think we should backport this to 11.3.x

Actually lets not fix in 11.3x unless someone thinks it is worth it.

This is broken in the UI only.
I used git bisect to identify this issue.
This breaks install with:

Symfony\Component\DependencyInjection\ContainerBuilder->createService() (Line: 632)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet() (Line: 1315)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1267)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1167)
Symfony\Component\DependencyInjection\ContainerBuilder->createService() (Line: 632)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet() (Line: 1315)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1267)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1794)
Symfony\Component\DependencyInjection\ContainerBuilder->callMethod() (Line: 1223)
Symfony\Component\DependencyInjection\ContainerBuilder->createService() (Line: 632)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet() (Line: 1315)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1267)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1167)
Symfony\Component\DependencyInjection\ContainerBuilder->createService() (Line: 632)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet() (Line: 1315)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1267)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1167)
Symfony\Component\DependencyInjection\ContainerBuilder->createService() (Line: 632)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet() (Line: 1315)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1267)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1167)
Symfony\Component\DependencyInjection\ContainerBuilder->createService() (Line: 632)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet() (Line: 1315)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1267)
Symfony\Component\DependencyInjection\ContainerBuilder->doResolveServices() (Line: 1167)
Symfony\Component\DependencyInjection\ContainerBuilder->createService() (Line: 632)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet() (Line: 577)
Symfony\Component\DependencyInjection\ContainerBuilder->get() (Line: 28)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (Line: 100)
Drupal\Core\Utility\CallableResolver->getCallableFromDefinition() (Line: 688)
Drupal\Core\Extension\ModuleHandler->getHookImplementationList()
array_map() (Line: 437)
Drupal\Core\Extension\ModuleHandler->getCombinedListeners() (Line: 418)
Drupal\Core\Extension\ModuleHandler->alter() (Line: 117)
Drupal\Core\Entity\EntityTypeBundleInfo->getAllBundleInfo() (Line: 80)
Drupal\Core\Entity\EntityTypeBundleInfo->getBundleInfo() (Line: 374)
Drupal\Core\Entity\EntityFieldManager->getFieldDefinitions() (Line: 1214)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->loadFromDedicatedTables() (Line: 503)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->mapFromStorageRecords() (Line: 428)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->getFromStorage() (Line: 394)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->doLoadMultiple() (Line: 340)
Drupal\Core\Entity\EntityStorageBase->loadMultiple() (Line: 267)
Drupal\Core\Entity\EntityStorageBase->load() (Line: 277)
Drupal\Core\Installer\Form\SiteConfigureForm->submitForm() (Line: 108)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 45)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 624)
Drupal\Core\Form\FormBuilder->processForm() (Line: 347)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 964)
install_get_form() (Line: 613)
install_run_task() (Line: 566)
install_run_tasks() (Line: 123)
install_drupal() (Line: 53)

It ends up with only 59 database tables too.

Reopening for now, if we need a new separate issue let me know.

MR with the fix: https://git.drupalcode.org/project/drupal/-/merge_requests/14770
Changing the renderer and library dependency resolver arguments to service closures prevents loading the kernel as a dependency during install. I'm not sure why the error did not show in StandardInstallerTest, but I won't be able to look into tests till later.

For now:

(on main) test by installing Drupal Standard (or Umami) via UI at core/install.php and observe error
Screenshot of what the error in the installer looks like:

(on MR branch) error does not appear and user is redirected to front page.

Thinking about this a little more: we maybe should investigate whether kernel should still be a synthetic service at this point in the install process. Changing the arguments in the hooks class to service closures avoids the error, but it might surface again in other hooks classes.

In which case, maybe it does make more sense to close this issue again and open a new one?

Did investigation and it really should be a new issue: #3573856: SiteConfigureForm properties need to be reset after module install in submitForm()