Add filters to allow themes and plugins to pass HTML attributes to different Nav Walker output

[davidwebca]

Currently, Nav Walkers allow custom HTML attributes to be passed to the link (<a>) element only. There are many instances, like when creating a menu with a declarative JavaScript library, where it would be ideal to be able to pass down HTML attributes to any of the built element. UL and LI are currently excluded of this.

Here's a link to the pull request: ​https://github.com/WordPress/wordpress-develop/pull/3645

Currently, Nav Walkers allow custom HTML attributes to be passed to the link (<a>) element only. There are many instances, like when creating a menu with a declarative JavaScript library, where it would be ideal to be able to pass down HTML attributes to any of the built element. UL and LI are currently excluded of this.

Trac ticket: https://core.trac.wordpress.org/ticket/57140

[danyk4]

this is very useful ticket
in modern web we need to add custom attrs to the wp menu

Thanks @costdev for your pointers, very helpful.

As for the HTML attributes building function, here's my train of thought:

At first, I started using the ​code that was already there with the assumptions that it had already been approved by core and had the least risk of causing breaking changes for end users. I had replicated the code multiple times and it started feeling silly so I extracted it into its separated protected function in the Nav Walker's class. But, then, my research lead me to previous efforts regarding this:

• A general HTML attribute building function just like the one I extracted that could be added to formatting.php - https://core.trac.wordpress.org/ticket/23236. Opened 10 years ago and closed 9 years ago for lack of interest.
• A general HTML attribute name escape function with an interesting conversation about the differences in allowed characters to escape or not in attribute names versus values. https://core.trac.wordpress.org/ticket/43010

I think it's safe to assume that the path to have this included as fast as possible without introducing breaking changes in Nav Walkers would be to continue with this patch without involving too many different conversations at once, but once that's done, we could start looking at reviving those two conversations, especially since the second one already has a patch and is just sitting there without activity. It would be amazing to have a general HTML attributes builder functions that every classes could refactor to use and eventually debloat + secure a lot of code.

@davidwebca Thanks for the detailed response!

I agree with you that given the following:

1. The existing code doesn't escape attribute names.
2. Extenders may be using spec-valid attribute names that esc_attr() would break.
3. Core doesn't have a spec-valid escape for attribute names.

This PR shouldn't escape the attribute names, doesn't introduce an additional security issue here, and escaping these attribute names should be part of wider discussion about the introduction of a spec-valid escape for attribute names.

Had this been newly introduced code, the introduction of a spec-valid escape in future would be an _enhancement_, not a regression. However, using esc_attr() in this PR would be a regression.

---

Regarding test failures, note the previous output:
{{{php
$output .= $indent . '<li' . $id . $class_names . '>';
}}}

Reversing lines 192 and 193 in the PR shows all tests passing with:
{{{sh
./vendor/bin/phpunit --filter '/Tests_Menu_Walker_Nav_Menu|Tests_Menu_wpNavMenu/'
}}}

This PR shouldn't escape the attribute names, doesn't introduce an additional security issue here, and escaping these attribute names should be part of wider discussion about the introduction of a spec-valid escape for attribute names.

Just a quick note that WordPress prevents double escaping, so esc_attr() should be fine in this context. From wp shell to demonstrate:

wp> $attr = 'Pens & Pencils';
=> string(14) "Pens & Pencils"
wp> $attr = esc_attr( $attr );
=> string(18) "Pens & Pencils"
wp> $attr = esc_attr( $attr );
=> string(18) "Pens & Pencils"

@peterwilsoncc So you don't anticipate any issues causes by introducing esc_attr() for attribute names in this PR?

If so, should this change be made in this PR, or a follow-up ticket to note the change?

@costdev No, I don't. I'm happy for the change to remain in this PR, the data is currently escaped and this PR is simply moving around where that takes place. Feel free to correct me if I am misunderstanding, I've only done a quick scan of the PR.

Summary of discussion with ​@peterwilsoncc in Slack:

• ​The existing code doesn't escape attribute names, only values.
• Introducing esc_attr() to attribute names here could break CSS styling.
• Further consideration is needed before escaping attribute names in already-existing code.

In short: The PR can continue without introducing esc_attr() for attribute names.

I've made the swap suggested by @costdev and tests seem to be okay.

Hi @costdev! Just wanted to check on this. Is there anything else I can do to help get the ball rolling or is this just waiting to be pulled in?

[costdev]

Thanks for the ping @davidwebca!

Aside from some unit tests to ensure the new filters are run, and only when they should run, this looks close to being ready for commit consideration.

As this is an enhancement, and we're in Beta for 6.2, I'll milestone this ticket for 6.3. We should be able to land this next cycle.

[davidwebca]

Amazing thanks!