Contrib.social

Overriding already overridden libraries-override requires knowledge of previous libraries-overrides

Open on Drupal.org →
Created on 30 December 2015, almost 9 years ago
Updated 19 April 2023, over 1 year ago

Problem/Motivation

A bug in libraries-override #2451411: Add libraries-override to themes' *.info.yml → makes it difficult to override a library or library asset that has already been overridden. You have to know the theme which last overrode a library and then use the full path to the overriding theme asset. This presents a WTF to themers and makes it difficult to reuse theme code. It also makes the sub-theme dependent on the internal file structure of the base theme.

In normal circumstances overriding an already overridden library would be and edge case, but with the introduction of the Stable base theme, a lot of system libraries have already been overridden. This also implies that possible future changes in the asset file structure of Stable could break sub-themes that override libraries already overridden by Stable.

Workaround

The current workaround is explained in #2642122-4: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides → and #2642122-18: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides → .

Proposed resolution

1. Fix the bug so that the libraries-override declaration should use the same keys as the libraries declaration in the original libraries.yml file.
2. Maintain a BC layer so that themes that have already used the workaround will not break immediately. Remove the BC layer in D10 ( #2852314: Remove BC layer for libraries-override that is already overridden → )

Remaining tasks

Reviews
Commit

User interface changes

None

API changes

Method signatures of 3 protected methods in LibraryDiscoveryParser are changed

Data model changes

None.

🐛 Bug report
Status

Needs work

Version

10.1 ✨

Component
Theme  →

Last updated 4 days ago

Created by

mbkaba22

Live updates comments and jobs are added and updated live.
  • CSS

    It involves the content or handling of Cascading Style Sheets.

  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇮🇹Italy finex

    Hi, patch #151 also works fine on Drupal 10. Thank you very much!

  • Comment 6 months ago →
    🇳🇱Netherlands Betawerk

    Making the patch compatible for 10.3.x

  • Comment 6 months ago →
    🇵🇱Poland sokolff Wroclaw

    I can confirm that the patch from #154 works with Drupal 10.3.0.

  • First commit to issue fork.
  • Comment 30 days ago →
    🇬🇧United Kingdom scott_euser
    1. Started converting to MR
    2. Updated some tests but more still fail, e.g. phpunit -c core/phpunit.xml core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php --filter=testBaseThemeLibrariesOverrideInSubTheme
    3. Updated issue summary to target D12 for removal of BC layer
  • Merge request !10292#2642122: Convert patch to MR, start fixing test coverage → (Open) created by scott_euser
  • Pipeline finished with Failed
    30 days ago
    Total: 118s
    #346836
  • Comment 29 days ago →
    🇬🇧United Kingdom scott_euser

    Okay tests still failing, and I think its because maybe we still have a flaw here.

    The commenting around the BC change in LibraryDiscoveryParser::setOverrideValue() is a bit unclear to me. It seems like it wants to check for the workaround like

    libraries-override:
  contextual/drupal.contextual-links:
    css:
      component:
        /core/themes/stable/css/contextual/contextual.module.css: false

    So to me that means it should be checking if the override like /core/themes/stable/css/contextual/contextual.module.css is from the base theme of the current theme (ie, direct parent theme, not grandparent), and if so, then throw the deprecation, but continue to work.

    Is that the correct intention? If so I think its not working like that quite yet. Happy to dig a bit but wanted to check if that's the right direction first.

    Thanks!

  • Pipeline finished with Failed
    29 days ago
    Total: 254s
    #347479
  • Pipeline finished with Failed
    29 days ago
    Total: 777s
    #347487
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024