Attribute hreflang not allowed on element li at this point

Re-roll #113 for 10.0.x

Re-roll #116 against 10.1.x and 11.x. The custom commands failure from #116 should have been fixed.

The last submitted patch, 118: 2454289-118-10.1.x.patch, failed testing. View results →

The last submitted patch, 118: 2454289-118-11.x.patch, failed testing. View results →

I've re-read through every comment in this issue and have observed the following:

data-drupal-link-system-path

in #4, @nod said

li element shouldn't have the data-drupal-link-system-path either.

in #94, @andrewmacpherson said

What's going on with the <li data-drupal-link-system-path> attribute? In #4, _nod suggested removing it from the list item. It's still there though. It looks like an earlier patch (#26) removed it, but by patch #82 it's come back.

This hasn't been addressed, so we need to decide whether this is something that needs to be done here or not, and update the issue summary and description accordingly.

data-drupal-language

in #94 @andrewmacpherson also said

The new data-drupal-language attribute isn't very well explained.

and this hasn't been addressed yet either.

failing tests in #118 need addressing

needs re-rolling

Here's the re-rolls for 10.3.x and 11.x - both based on #118 🐛 Attribute hreflang not allowed on element li at this point Needs review

The other items from #121 🐛 Attribute hreflang not allowed on element li at this point Needs review remain outstanding.

Created a merge request and adjusted the unit test.

I think changing data-drupal-link-system-path is out of scope. This was added on purpose in #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links → and the concept of having the active class on li links has existed even before that. It's not broken and doesn't cause problems.

Not sure what/where to document on data-drupal-language, it's an internal attribute, like the other two, I don't think they are documented either.

Have not reviewed yet but issue summary could use some love

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Updated the issue summary. Didn't mention the discussion around the other data attributes, I don't really see how that's in scope. This doesn't change them in any way and we afaik specifically support active classes on list items.

Hiding patches as the fix is in the MR,

Thanks for updating the issue summary.

Applying the MR and enabling the translation modules I am seeing the change being made see screenshots.

Before

After

Don't see any open threads or pending items right now. And nothing seems off in the code.

Rebased this, had to adjust script size. The diff is a bit misleading because other changes were already causing it to be very close to the upper limit of the 500 +/- range, the actual value before was 170941, now it is 171197 (which I rounded up to 171200), so a difference of 256.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.