Excluded paths are not valid for multilingual sites that have language prefix.

Created on 1 November 2023, about 1 year ago
Updated 15 December 2023, 11 months ago

Problem/Motivation

Static generation don't match urls that have language prefix compared to the predefined excluded paths.

Steps to reproduce

Generate static for multilingual site that have language prefix on a default language.

Proposed resolution

Add all language prefixes to the excluded paths array.

Remaining tasks

Review and test

🐛 Bug report
Status

RTBC

Version

1.11

Component

Code

Created by

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

Comments & Activities

  • Issue created by @NikolaAt
  • Status changed to Needs review about 1 year ago
  • Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Status changed to Needs work 11 months ago
  • 🇬🇧United Kingdom joachim

    Looks ok, just a few nitpicks:

    1. +++ b/modules/tome_static/src/EventSubscriber/ExcludePathSubscriber.php
      @@ -71,6 +71,17 @@ class ExcludePathSubscriber implements EventSubscriberInterface {
      +    if (\Drupal::languageManager()->isMultilingual() && \Drupal::config('language.types')->get('negotiation.language_interface.enabled.language-url')) {
      

      Services should be injected.

    2. +++ b/modules/tome_static/src/EventSubscriber/ExcludePathSubscriber.php
      @@ -71,6 +71,17 @@ class ExcludePathSubscriber implements EventSubscriberInterface {
      +      $languageNegotiation = $config->get('url.prefixes');
      

      The variable name doesn't match the data. Core tends to have: $prefixes = $config->get('url.prefixes');

    3. +++ b/modules/tome_static/src/EventSubscriber/ExcludePathSubscriber.php
      @@ -71,6 +71,17 @@ class ExcludePathSubscriber implements EventSubscriberInterface {
      +        foreach ($languageNegotiation as $lang) {
      

      $lang should be $langcode.

  • Status changed to Needs review 11 months ago
  • Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7
    last update 11 months ago
    Waiting for branch to pass
  • @joachim,
    Adding patch with variable name changes. Looked into dependency injection point but getExcludedPaths() is a static method and we won't be able to use $this inside static method. If we make it non-static, then we will have to modify all code where this method is being called.

    Your feedback is appreciable.

  • 🇬🇧United Kingdom joachim

    Ah I hadn't seen it was static. That's because it's called like this:

    $paths = ExcludePathSubscriber::getExcludedPaths();
    

    from StaticGenerator. Which is just ICK but out of scope for this issue.

  • Status changed to Needs work 11 months ago
  • 🇬🇧United Kingdom joachim

    I couldn't find anywhere in core that actually documents this, but AFAICT the prefixes config is $langcode => $prefix.

    So the loop variable should be $prefix.

    Also, it can be empty! In that case, you'd be incorrectly adding a second slash at the front of the path.

  • Status changed to Needs review 11 months ago
  • Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7
    last update 11 months ago
    Waiting for branch to pass
  • Attaching new patch with your feedback.

  • Status changed to Needs work 11 months ago
  • 🇬🇧United Kingdom joachim

    Nearly there! :)

    +++ b/modules/tome_static/src/EventSubscriber/ExcludePathSubscriber.php
    @@ -71,6 +71,17 @@ class ExcludePathSubscriber implements EventSubscriberInterface {
    +          $excluded_paths[] = !empty($prefix) ? "/" . $prefix . $path : $path;
    

    If $prefix is empty, there's no need to add $path, it's already in the array.

    Also, nitpick, but the / can be single-quoted.

    Another nitpick: would be nice to have a comment at the top of the section to explain we're adding language-prefixed versions of all the paths.

  • Adding new patch with addressed feedback.

  • Status changed to RTBC 11 months ago
  • Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7
    last update 11 months ago
    Waiting for branch to pass
  • 🇬🇧United Kingdom joachim

    LGTM!

Production build 0.71.5 2024