Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Hreflang links were pointing to the actual node view page ("node/9") instead of just front ("/") when using that node as a front page (google sent a notification about the issue).
Proposed resolution
Don't create hreflang links from the entity route, just add the languages prefix to root path.
Comment | File | Size | Author |
---|---|---|---|
#46 | problematic_hreflang_links-2796399-46.patch | 3.19 KB | criz |
#43 | after.png | 215.32 KB | abhisekmazumdar |
#43 | before.png | 167.85 KB | abhisekmazumdar |
#42 | interdiff_34-42.txt | 843 bytes | sarvjeetsingh |
#42 | problematic_hreflang_links-2796399-42.patch | 3.49 KB | sarvjeetsingh |
Comments
Comment #2
heikki CreditAttribution: heikki at Avoltus Oy commentedComment #4
caspervoogt CreditAttribution: caspervoogt at Plethora commentedI tested the patch ant it works great for me.
Comment #5
caspervoogt CreditAttribution: caspervoogt at Plethora commentedActually, I noticed one problem. It's spitting out https:// for me, rather than http:// which is my current request protocol. My site has no SSL on it, though a shared SSL certificate is available. I have written a patch that can be applied after hreflang_front_page-2796399-2.patch (from #2) to adjust the logic. I have not tested this on an https site, but it works for my http site, in any case. Maybe someone can test it on SSL?
If this works, we can reroll so it can be applied cleanly with a single patch.
Comment #6
caspervoogt CreditAttribution: caspervoogt at Plethora commentedComment #8
caspervoogt CreditAttribution: caspervoogt at Plethora commentedMy patch failed testing but it is no surprise since mine was a follow to the initial patch. We'll need to re-roll later.
Comment #10
heikki CreditAttribution: heikki at Avoltus Oy commentedRerolled the patch for 8.3.x and used caspervoogts way of determining the protocol.
Comment #11
heikki CreditAttribution: heikki at Avoltus Oy commentedComment #12
pritish.kumar CreditAttribution: pritish.kumar at OpenSense Labs commentedI have uploaded the patch by improving the coding standard issues. I hope it's correct
Comment #14
pritish.kumar CreditAttribution: pritish.kumar at OpenSense Labs commentedComment #19
User Advocate CreditAttribution: User Advocate commentedRe-rolling #10 which is the last working patch. I've made the adjustments that were intended in #12 and #14 and done a bit of cleanup.
With regard to the adjustments made in #10 for determining the protocol, I've removed that. Based on my local tests that appears to have been taken care of elsewhere in core but unfortunately I can't say exactly where. This patch applies to 8.7.x and 8.6.x.
Also, this related issue presents a wider perspective and could lead to a different solution to the problem: https://www.drupal.org/project/drupal/issues/2994800
Comment #21
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedPatch #19 worked for me. AFAIC this is RTBC.
But given the related issue, I wonder if I should change the status or not.
Comment #23
Piotr PakulskiPatch #19 worked for me on 8.8.x.
botanic_spark I would go with this and merge it?
Comment #24
Krzysztof DomańskiI added a test and updated the issue summary.
Comment #27
Krzysztof DomańskiComment #28
pratik_kambleComment #29
Krzysztof DomańskiNeeds review.
Comment #30
Krzysztof DomańskiNeeds a re-roll.
Comment #31
mayurjadhav CreditAttribution: mayurjadhav at Srijan | A Material+ Company commentedWorking on it, Assigning this to me.
Comment #32
mayurjadhav CreditAttribution: mayurjadhav at Srijan | A Material+ Company commentedRerolled #27 patch, Kindly review.
Comment #33
Kristen PolThanks for the update. Tiny nitpick:
Exceeds 80 chars. Consider rewording so it doesn't spill to 3 lines, e.g.
(You should confirm that fits within the 80 chars because a "the" was added to the first line.)
Comment #34
mayurjadhav CreditAttribution: mayurjadhav at Srijan | A Material+ Company commentedThanks @Kristen Pol
here us the updated patch along with interdiff.
Comment #35
Kristen PolThe interdiff looks good, thanks. Haven't had a chance to manually test it yet.
Comment #36
Ruuds CreditAttribution: Ruuds at Groowup Digital Agency commentedThe patch from #34 works great for me.
Comment #37
Kristen PolManually tested with and without patch and it works as expected.
Without patch
With patch but without front page set to node/2
With patch but with front page set to node/2
Comment #38
Kristen PolI just read all the comments above and it wasn't clear to me that anyone actually reviewed the code. Seems like people were just testing that it worked. I only did a superficial review before so I'll see if someone on the bugsmash team can do a better code review.
Comment #39
maacl CreditAttribution: maacl commentedChanges and Test look good to me. Patch works as intended.
This is something SEO tools give a warning about, so it would be nice to get this in.
Comment #40
catchthe patch looks good but there are a couple of issues:
This needs to be added to the cspell dictionary:
Comment #41
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedComment #42
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedI have made the changes as suggested above and updated the patch.
please review, thanks!
Comment #43
abhisekmazumdarHi thanks @sarvjeetsingh for patch.
I applied the patch on 9.1.x.
Before patch:
After applying the patch:
The points said by @catch are covered on the latest patch.
Comment #45
catchCommitted 11118a3 and pushed to 9.1.x. Thanks!
Comment #46
crizHere is a reroll of the patch for 8.9.x, so that it applies for 8.9.6.
Comment #47
Piotr Pakulski#46 reroll works fine for me for 8.9.6