Problem/Motivation

system_page_attachments() is a hook implementation but it's called from the theme system.

We should extract anything that's absolutely required from it, and move it to a helper method somewhere. system_page_attachements() can then call that method, and all the places that call system_page_attachments() can drop their hard-coded dependency on system module.

Steps to reproduce

Proposed resolution

Add BareHtmlRenderer::addAttachments() and call this from system_page_attachments()
Deprecate the system/base library and move it to core.libraries.yml

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3428339

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !7081

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs work

A few things going on here:

- the system/base library shouldn't be in system module at all, moved to core.libraries.yml and core/misc.

- we probably need to do the same for system.admin and system.maintenance since these are called by template_preprocess_maintenance_page(), but that might not be necessary to fix this specific issue and dependencies, potentially can happen in a follow-up.

andypost made their first commit to this issue’s fork.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs work » Needs review

Tests are green. I implemented bc for anything declaring system/base as a library dependency but not for library overrides - personally I think a change record should be enough for those given it's essentially an alter hook.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

- the system/base library shouldn't be in system module at all, moved to core.libraries.yml and core/misc.

I bet system/base precedes the "core extension" existing! 👴😄

All those core/modules/system/** to core/misc/** moves … break BC AFAICT? 🫣 We can move the asset library for sure, but I'm not sure that we can also move the underlying files? 🤔

Ah, you specifically commented on that in #5. Well … if a release manager thinks that's okay, then I'm fine with it too — you're totally right that it's far less common to do library overrides. And the changes in claro.info.yml can serve as an inspiration for how to do this.

Just one remaining question regarding BC: how can a theme be compatible with both the "before" and "after" states in a single version?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Looking at system/base I found a tonne of issues which are now documented in #3432183: Move system/base component CSS to respective libraries where they exist and related issues. If we do those issues, there will be much less change to do here, so I think we should soft-postpone this issue to avoid moving things two or three times.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Postponed

Postponing this on #3432183: Move system/base component CSS to respective libraries where they exist #2880237: [meta] Refactor system/base library, we might even be able to just drop the calls altogether if we do those without having to manually load the libraries in the installer any more, or at least, it would be a very short list.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

catch’s picture

catch
he/him
Primary language English
commented
Status: Postponed » Active

This can probably be tackled now, albeit slightly imperfectly.

BareHtmlRenderer::systemPageAttachments() we have the following:

    // \Drupal\Core\Theme\ThemePreprocess::preprocessMaintenancePage().
    $page['#attached']['library'][] = 'system/base';
    if (\Drupal::service('router.admin_context')->isAdminRoute()) {
      $page['#attached']['library'][] = 'system/admin';
    }

If we move that to system module's PageAttachmentsHook::pageAttachments() then the tight coupling between system module and BareHtmlRenderer is dropped.