Custom breadcrumbs disappear after 2.0.8

oph. Thanks for the report. We'll take a look as soon as we can. Clearly we need better test coverage for this feature.

Appears to be caused by https://git.drupalcode.org/project/easy_breadcrumb/-/commit/e4109f3f4851... (issue #3271576). Now instead of trimming leading *and* trailing slashes from $path, it only removes trailing slashes.

This conflicts with line 321 where the $custom_path is trimmed on both sides.

So we run into a scenario where they don't match:

$path: /path-1
$custom_path: path-1

We could make line 321 only trim spaces from both sides, and then add an rtrim below for the slashes so it matches the behavior of $path, but that would be a breaking change if someone has a custom path config with multiple leading slashes. (unlikely, but possible).

I think it's reasonably safe to assume someone doesn't have a custom path with multiple leading slashes.

It's off topic, but I work for our library. Thanks for all the great work y'all do at City of Portland. Feel encouraged to open an MR with what you think the best fix is here.

Howdy neighbor! Appreciate your work as well.

I did some more digging on this, I think #3271576 (the source of this regression) was actually a "duplicate" bugfix. The same bug was fixed on Feb 3, 2022 in #3262378 with a different approach in `getRequestForPath` of adding a leading slash if it doesn't exist.

That fix didn't make it into a release (v2.0.3) until Jun 5, 2022. In the meantime, on Mar 24, 2022, #3271576 was opened. In July 2022, a couple users in the comments appear to have the same issue, I suspect due to an outdated module version. This bug should be theoretically impossible with v2.0.3, released a month prior.

This ultimately leads to a redundant bugfix being merged and released in v2.0.8.

Anyway. Sorry for the unprompted history lesson. I submitted an MR that:

1. Adds a note to the help text that "Use the real page title when available" needs to be enabled to use the <title> placeholder. This appears to have been a missed followup from #3271576.
2. Reverts the change that caused this regression.
3. Fixes another bug I found during testing where the title placeholder doesn't get trimmed, by doing the trimming and XSS filtering *after* the title/regex replacements are done.
4. Refactors TitleResolver so it skips all of the unnecessary entity loading if the "alternative title field" is not enabled. This should be a small performance boost.
5. Adds tests for this issue and #3271576 to prevent future regressions.

Found an issue with the alternative title logic. Let me take that MR back for a bit.

The merge request patch is working for me with easy_breadcrumb 2.0.8. I appreciate all the background information, too!

My only question is on this line:

- $title = Html::decodeEntities(Xss::filter(trim($settings[0])));

Are we still escaping JavaScript if we remove that line?

Also! Great freaking work on this merge request. Tests and everything.

Are we still escaping JavaScript if we remove that line?

Good question, it wasn't completely removed but just moved lower down.

So yes, it will still do the XSS filtering, but now after the placeholder title/regex replacements are done. I moved it because I noticed when using the placeholder, it wasn't getting filtered or trimmed.

Awesome couldn't tell for certain the Diff, thanks for the extra info!

Code looks good. Needs some more manual testing still. We'll wait for Spuky to take a look.

~G

I manually tested and can confirm this patch resolves the issue described. Tested on a clean install with easy_breadcrumb 2.x and core standard install 10.3.7-dev.

* Create a basic page, with title Page 1 and alias `/test`
* View page and confirm breadcrumbs are Home -> Page 1
* Update easy_breadcrumbs config—add Paths to replace with custom breadcrumbs: `/test :: Capybara | /test`
* Confirm breadcrumbs are unchanged
* Apply patch and clear cache
* Confirm breadcrumbs now correctly display Home -> Capybara
Without patch:

With patch:

Fantastic improvements, merging.