- Issue created by @NikolaAt
- Status changed to Needs review
about 1 year ago 3:30pm 1 November 2023 - Open on Drupal.org →Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7last update
about 1 year ago Waiting for branch to pass - Status changed to Needs work
11 months ago 11:38am 13 December 2023 - 🇬🇧United Kingdom joachim
Looks ok, just a few nitpicks:
-
+++ 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.
-
+++ 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');
-
+++ 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 2:27pm 13 December 2023 - Open on Drupal.org →Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7last update
11 months ago Waiting for branch to pass - 🇮🇳India chandreshgiri gauswami
@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 3:22pm 13 December 2023 - 🇬🇧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 1:47pm 14 December 2023 - Open on Drupal.org →Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7last update
11 months ago Waiting for branch to pass - Status changed to Needs work
11 months ago 2:09pm 14 December 2023 - 🇬🇧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.
- Status changed to RTBC
11 months ago 10:30am 15 December 2023 - Open on Drupal.org →Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7last update
11 months ago Waiting for branch to pass