can no longer format links using filters because the output is now escaped.

[adampatterson]

First off, I have used this class for a few years and love it! This isn't technically a bug. But I recently updated to 4.3 and had some unexpected results.

I use a function that lets me format my titles using basic markdown. After updating from 4.1 to 4.3 I noticed it stopped working.

add_filter('the_title', [$this, 'markdown_title'], 1);

I noticed the change here https://github.com/wp-bootstrap/wp-bootstrap-navwalker/blob/master/class-wp-bootstrap-navwalker.php#L250

That escapes the output which is obviously a good thing to do. But it also limits the control I have over my site...

Previously

Currently

There is a PR for #457 which looks like it removes the escape but also adds some style classes that I do not fully understand.

I would like to pass property through to the walker during setup which I can do.

'escape_titles'  => false,

[IanDelMar]

Related to #445

[adampatterson]

@IanDelMar I made a pull request that includes that optionally allows the user to disable esc_html

I thought it was a better solution than simply removing it altogether since I would imagine this class is used in many different applications.

[pattonwebz]

Hey, thanks for working on this. At the moment my time is so tight I just can't make any for working on this project however if you are able to test things confirm they work etc I would be more than happy to merge changes and roll things in.

Sorry for the lack of assistance here from my side.

[adampatterson]

@pattonwebz No problem,

I have tested it locally, and it worked perfectly.

I am not sure how your automated tests work but I can look and see if there is a relevant test to use.

Cheers!

[pattonwebz]

The automated tests really need improving/fixing so don't worry too much about those. I'll look the PR over quickly and see if I can merge it. @IanDelMar I know you have PRs and issues open too that I should look to merge. Feel free to ping me in those to look at them and I will try my best to look and merge this week.

Again sorry for such long delays everyone.

[IanDelMar]

@IanDelMar I made a pull request that includes that optionally allows the user to disable esc_html

Yeah, I did see that. I just wanted to add a cross-reference to make it easier to follow multiple issues/PRs adressing the same issue.

I thought it was a better solution than simply removing it altogether since I would imagine this class is used in many different applications.

The esc_html has been added in v4.3.0 realeased in October 2019. So I guess a large fraction of applications have not updated to v4.3.0 (yet). As you have to come to this repository and have to actively grab the file and place it somewhere in your theme's directory there's a chance that those who update frequently will check the changes that have been made. So I think it's pretty safe to just remove it. But yeah, it means a "breaking" change.

If someone cares about escaped titles then the best way to achieve this is to use the the_title hook and to do it themselves. Even if the Nav Walker does default escaping it should use that hook. Consider your PR and me trying to have an escaped title with HTML. First I woud have to turn off the default escaping in the corresponding template file (most likely the header.php) as it provides the the_title hook with an escaped string (using esc_html()! not for example wp_kses()) making my escaping preference (preserving HTML) impossible. And second I have to either add a filter in my functions.php or change the Nav Walker's code. If the Nav Walker would use the hook I just have to add remove_filter() and add my own filter in my functions.php. I think this is a much cleaner solution and I'm pretty sure WordPress gave us all the hooks to use them! Besides this I still have a strong preference to stay as close as possible to the default Nav Walkers behaviour. Simply because that's what people expect.

@pattonwebz No worries, I have not expected you to have a closer look at my PRs before looking whether to incorporate those changes in v5.

[adampatterson]

@IanDelMar I think that makes sense, I was looking for a way to preserver that change made while still allowing for the flexibility that was previously there.

I am not too familiar with all the nav hooks, but my markdown function was called through a few filters.

            add_filter('the_title', [$this, 'markdown_title']);
            add_filter('widget_title', [$this, 'markdown_title']);
            add_filter('single_post_title', [$this, 'markdown_title'], 8);

You are saying that if the user wanted to escape titles they would also call apply_filters('the_title', esc_attr($item->title), $item->ID); or something to that effect.

[IanDelMar]

You are saying that if the user wanted to escape titles they would also call apply_filters('the_title', esc_attr($item->title), $item->ID); or something to that effect.

No. The menu item title is not an attribute. I highly doubt that users will use esc_attr(). It's more reasonable that they will use esc_html() as it is done in the current walker version. That may make perfectly sense, if you are the user of your theme and you know there won't be any HTML in the menu titles.

If a user (of the Nav Walker) wanted to escape titles then in my opinion the most plausible way is to

add_filter( 'the_title', 'my_title_filter' );
function my_title_filter( $title ) {
  return some_escaping_function( $title );
}

But apply_filters('the_title', esc_html($item->title), $item->ID) breaks this (I won't refer to esc_attr()). The the_title hook is used for both post titles and menu item titles. Using the hook gives consistent escaping and looks of the titles. WordPress allows for HTML in post titles and according to the Theme Unit Data it does demand HTML in titles to render correctly - at least some HTML tags. While there is no statement on HTML tags in nav menu titles, one can assume it's either to render HTML tags correctly (as with post titles) or to remove the tags as it is required for browser windows / tabs (For the latter use the nav_menu_item_title hook, because you want to keep the HTML in your post titles). Both cases are not met with esc_html(). And what's even worse you can't just apply e.g. wp_kses() or wp_strip_all_tags() using the_title or the nav_menu_item_title hook because there are no more HTML tags in the menu item title once it passes esc_html().

Also, the Nav Walker is meant to be part of a theme. So what is allowed in nav item titles should be up to the theme developer, shouldn't it? As a theme developer I would expect my hooked escaping function to work and would expect HTML being rendered correctly. My expectations are met by the default Nav Walker - most likely because that's where my expectations are coming from.

My conclusions: I think it's not neccessary to escape menu titles in this specific case. As demonstrated to do so can have adverse effects. If the option to escape is provided it should default to false as this is the default WordPress Nav Walker behaviour. If the option to escape is provided it should be easy to change the way escaping takes place by either using a native WordPress hook or by providing a custom hook and not forcing devs to change the Walker's code.

Furthermore, whether (all) titles should be escaped by default (I may even support this) seems to be more of a WordPress Core issue than a wp-bootstrap-navwalker issue. For now, the answer is no.

[IanDelMar]

As of #445 titles are no longer escaped.