Fix node and taxonomy entity template 'url' variables

+++ b/language_hierarchy.module
@@ -84,3 +84,35 @@ function language_hierarchy_form_language_admin_edit_form_builder($entity_type,
+    'language' => Drupal::service('language_manager')->getCurrentLanguage(),

The global Drupal class should be accessed with a preceding slash, e.g. see core's node_form_system_themes_admin_form_submit():

 \Drupal::service('router.builder')->setRebuildNeeded();

If we're going to cover the image formatter, we should also cover the responsive image formatter in the same way. However, I don't see $variables['url'] being set in template_preprocess_image_formatter(), can you point me to where the language is getting set incorrectly for that?

I also wonder whether this is all actually a core bug anyway? Why should core be using the entity language for the URLs, rather than the current language? I'd be tempted to check for an existing core issue discussion about this or start one rather than implement this here, since I'm not sure it's actually specific to language_hierarchy.

Here's a patch that:

1) Re-rolls, since the last version didn't apply. To make that easier, I've also moved all the new functions at the bottom of the module file, rather than in the middle.
2) Replaced deprecated use of the url() method on entities.
3) Added the preceding slash to the use of the \Drupal class.
4) Covered responsive image formatter
5) Introduced a helper function that each preprocess function can use, in order to be more accurate. Each of the functions also checks that the relevant canonical link is actually being linked to. If any other module/theme decides to change the link to something other than what we expect core to have done, we don't really want to touch the links.
6) Added an option that allows other code to set to stop our code from replacing the link language.

There could be an issue with $variables['url']; which could be a string like '/en/node/9'.

+/**
+ * Implements hook_preprocess_node().
+ */
+function language_hierarchy_preprocess_node(&$variables) {
+  if (!empty($variables['url'])) {
+    /** @var \Drupal\node\NodeInterface $node */
+    $node = $variables['node'];
+    /** @var \Drupal\Core\Url $url */
+    $url = $variables['url'];
+    if ($url->getRouteName() === 'entity.node.canonical') {
+      $variables['url'] = language_hierarchy_fix_url_from_fallback($url, $node)
+        ->toString();
+    }
+  }

Since the previous patch is committed, the attached patch is intended only for this problem.

@J-Lee good catch, sorry about that, perhaps I was a little hasty in committing our patch.

What circumstances have set $variables['url'] to a string for you? I think language_hierarchy should stay out of the way entirely if $variables['url'] is anything unexpected like that.

@james.williams I can imagine that's a mistake somewhere in another module (or maybe theme). But I haven't found it yet.
I've saw this issue somewhere before in another module ...

Since $variables['url'] can be anything, this should be more fault tolerant.

I'm trying to find the cause of the problem. Then we can decide how to proceed.

You're right, we should be more fault tolerant and at least check $url is a Url :-) I'll wait to hear where it's being changed to a string though... if that's core or something else significant, I'm happy to work with strings as well as Urls.

We had to make use of the patch in #9 after the dev commit. It looks like per:

https://git.drupalcode.org/project/drupal/blame/8.8.x/core/modules/node/...
https://git.drupalcode.org/project/drupal/blame/8.8.x/core/modules/taxon...

The changes were made to both node and taxonomy on 23 December 2018 in the respective preprocess functions, where the $variables['url'] for both is cast to string. The check on if $url is a instance of Url makes sense.

Ah! My fault for working with a Drupal 8.7 site. My apologies, I'll get that resolved.

Thank you for @angrytoast.

Here is new patch which I have quickly created, checking all the $variables['url'] parts.

@J-Lee we cannot redeclare function check_url like that since it's already declared in common.inc. We can move this to to a helper class.

@Etroid You're right. The patch wasn't really tested by myself.

There are a few other things I'm not happy with.

• Name of the helper method
• What happens if there is no valid path?
• Documentation

Here is a patch with corrected method name, slightly better documentation and validity checks for the return value.

Sadly, \Drupal::service('path.validator')->getUrlIfValid($variables['url'])->toString() doesn't come out the same as $variables['url'], so I think we might have to go a bit further to confirm $variables['url'] is as expected. Otherwise, it's entirely possible that a different, unrelated, translation is being intentionally linked to, which LH shouldn't be overriding. I'm investigating...

Thanks all for your work on this.

Instead of using the path validator service, I've gone with a much simpler check like the following:

$variables['node']->toUrl()->toString() === $variables['url']

As far as I know, this is closer to what we actually intend and matches what core does.

I've then moved the route name & language checking into the language_hierarchy_fix_url_from_fallback() function, so the extra language_hierarchy_check_url() function isn't really needed.

I hope this works for everyone?

Cool. Didn't testet it yet.

After a quick review I saw the following:

+    $url = $variables['url'];
+    if (is_string($url)) {
+      $url_obj = $term->toUrl();
+      if ($url_obj->toString() === $url) {
+        $url = $url_obj;
+      }
+    }
+
+    if ($url instanceof Url) {
       $variables['url'] = language_hierarchy_fix_url_from_fallback($url, $term)

You can move the entire URL check to its own method and call it instead. Then you don't have to write the same thing several times.
All you have to do is insert a type hint on Drupal\Core\Entity\Entity\EntityBase. E.g. my_method($url, EntityBase $entity) {}

Thanks for the quick check.

The if ($url instanceof Url) check would still be needed if there's a type hint, so it was only a few lines that get repeated. Note that the preprocess image formatters don't have them, so the lines are only repeated once. So I hadn't felt it was worth refactoring into their own function. If further changes turn out to be needed, I'll look again, but I don't think it's worth it otherwise.

Yes, that's true.

I'll do some manual tests later.

We tested the patch for a few days in our test environment and found no side effects.

Thanks!