Problem/Motivation

Following #2467827: [META] W3C validation for Drupal Core, the W3C validator triggers an error for links generated for route . Unfortunately, the LinkGenerator.hp method adds the hreflang attributes in all cases while are made with tags which does not support the hreflang attribute.

The solution is to unset the hreflang in the same way it is done with href in the case of route.

CommentFileSizeAuthor
#8 interdiff-3186821-5-6_0.txt2.24 KBmohit_aghera
#8 3186821-test-only.patch2.4 KBmohit_aghera
#8 3186821-6_0.patch3.21 KBmohit_aghera
#6 interdiff-3186821-5-6.txt2.24 KBmohit_aghera
#6 3186821-6.patch3.21 KBmohit_aghera
#5 interdiff_3186821_3-5.txt988 bytesankithashetty
#5 3186821-5.patch993 bytesankithashetty
#3 remove_hreflang_on_span_button--3186821-2.patch944 bytesdom.

Issue fork drupal-3186821

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Dom. created an issue. See original summary.

dom.’s picture

dom. commented
Title: Attribute “hreflang” not allowed on element “span” at this point » Attribute “hreflang” not allowed on element “span” and “button” at this point
Status: Active » Needs review

Pushing a simple patch. No tests updates here to have a first pick at how many tests get eventually broken and needs fixing.

dom.’s picture

dom. commented
StatusFileSize
new remove_hreflang_on_span_button--3186821-2.patch944 bytes
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative, +Needs tests

Thanks for the issue and the patch.

1) Patch still applies cleanly to 9.2.

2) Although the code added is logical, stylistically, I would do the following instead:

unset($attributes['href'], $attributes['hreflang']);

3) Even if the logic stays as is, we need test coverage in LinkGeneratorTest.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
StatusFileSize
new 3186821-5.patch993 bytes
new interdiff_3186821_3-5.txt988 bytes

Updated patch in #3 addresing #4.2 . Retaining status "Needs work" for tests(#4.3). Thanks!

mohit_aghera’s picture

mohit_aghera commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 3186821-6.patch3.21 KB
new interdiff-3186821-5-6.txt2.24 KB

Adding test cases for basic validation.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Looking good, can we get a test-only patch here (that should fail)

mohit_aghera’s picture

mohit_aghera commented
StatusFileSize
new 3186821-6_0.patch3.21 KB
new 3186821-test-only.patch2.4 KB
new interdiff-3186821-5-6_0.txt2.24 KB

Adding test-only patch.
Reuploaded the previous patches again.
Here is the local run for test-only patch:

vendor/bin/phpunit -c core --filter="testGenerateNone"
PHPUnit 8.5.13 by Sebastian Bergmann and contributors.

Testing
F                                                                   1 / 1 (100%)

Time: 47.74 seconds, Memory: 808.00 MB

There was 1 failure:

1) Drupal\Tests\Core\Utility\LinkGeneratorTest::testGenerateNone
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<button type="button">Test With Language</button>'
+'<button hreflang="de" type="button">Test With Language</button>'
vendor/bin/phpunit -c core --filter="testGenerateNoLink"
PHPUnit 8.5.13 by Sebastian Bergmann and contributors.

Testing
F                                                                   1 / 1 (100%)

Time: 36.97 seconds, Memory: 808.00 MB

There was 1 failure:

1) Drupal\Tests\Core\Utility\LinkGeneratorTest::testGenerateNoLink
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<span>Test With Language</span>'
+'<span hreflang="de">Test With Language</span>'

Status: Needs review » Needs work

The last submitted patch, 8: 3186821-test-only.patch, failed testing. View results

mohit_aghera’s picture

mohit_aghera commented
Status: Needs work » Needs review

Changing to needs review as failures is from the test-only patch.

acbramley’s picture

acbramley commented
Status: Needs review » Reviewed & tested by the community

Looking great, fails as expected 🎉

  • catch committed 1b0a441 on 9.2.x 
    Issue #3186821 by mohit_aghera, Dom., ankithashetty, Kristen Pol:...

  • catch committed af95c84 on 9.1.x 
    Issue #3186821 by mohit_aghera, Dom., ankithashetty, Kristen Pol:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 1b0a441 and pushed to 9.2.x. Thanks! Also cherry-picked to 9.1.x.

Status: Fixed » Closed (fixed)

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