Contrib.social

Add library only if needed

Open on Drupal.org →
Created on 2 January 2025, 5 months ago

Problem/Motivation

https://git.drupalcode.org/project/navigation_extra_tools/-/blob/1.1.x/n...

/**
 * Implements hook_page_attachments().
 */
function navigation_extra_tools_page_attachments(&$variables) {
  $variables['#attached']['library'][] = 'navigation_extra_tools/icon';
}

The module loads a library which depends on navigation/internal.navigation, which is marked as internal by Core, and there is no check if the navigation toolbar is present.

So, currently it provokes the following bug 🐛 admin-toolbar-wrapper: Uncaught TypeError: doc.querySelector(...) is null Active .

Steps to reproduce

Enable navigation_extra_tools on a website, D10.3+ or D11.
Go on the website as anonymous user, see that the library is loaded.

Proposed resolution

Even if a module can't extend a library with its .info.yml like a theme, do the equivalent with a hook_library_alter instead of with a hook_page_attachements.

🐛 Bug report
Status

Active

Version

1.0

Component

Code

Created by

🇫🇷France Grimreaper France 🇫🇷

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Grimreaper
  • Comment 5 months ago →
    anish.ir

    Working on it.

  • Comment 5 months ago →
    🇩🇪Germany jurgenhaas Gottmadingen

    I wonder if this module should even remove the usage of that library. According to the comment in navigation.libraries.yml, this should not be done:

      # Internal library. Do not depend on it outside core nor add core usage
  # beyond the Navigation module.
  • Merge request !21Resolve #3497060 "Add library only" → (Merged) created by anish.ir
  • Pipeline finished with Success
    5 months ago
    Total: 361s
    #384657
  • Comment 5 months ago →
    anish.ir

    Hey,

    I have added the condition to attach the library using hook_library_alter instead of hook_page_attachments ( which was attaching it directly).

    Please have a look and let me know if there are any changes need to made here ( as suggested by @jurgenhaas )

    I am attaching a screenshot for console when a anonymous user visits the site, no error is present.
    Thank you.

  • Comment 4 months ago →
    🇧🇪Belgium svendecabooter Gent

    I can confirm that this patch fixes the bug described in 🐛 admin-toolbar-wrapper: Uncaught TypeError: doc.querySelector(...) is null Active .
    Also, without this patch the following files get added to our frontend theme, for anonymous users that do not see the navigation bar:
    - core/modules/navigation/css/components/admin-toolbar.css
    - core/modules/navigation/js/admin-toolbar-wrapper.js

    Code in these files interfered with our theme, which is resolved with this fix.

  • Pipeline finished with Success
    3 months ago
    Total: 193s
    #428878
  • Status changed to RTBC 2 months ago
  • Comment 2 months ago →
    🇮🇪Ireland lostcarpark

    Thanks for raising this. Clearly the library should only be loaded when the navigation is loaded, and certainly not for anonymous users.

    Merging.

  • Comment 2 months ago →
    System Message
  • Comment 2 months ago →
    🇮🇪Ireland lostcarpark

    Merged.

    There are a couple of warnings about deprecations for next minor/max PHP, but we'll take care of those in separate issues.

    @Jurgan, you make a good point about depending on the internal Navigation internal library. I think it would be worth looking at better ways to do that, but I think it should be in a follow-up issue.

  • Comment 2 months ago →
    System Message

    Automatically closed - issue fixed for 2 weeks with no activity.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024