Follow-up to #2480811: Cache incoming path processing and route matching

Problem/Motivation

Inbound and Outbound path processors are supposed to take a path that always has a leading /. The InboundPathProcessorInterface documents this fact. OutboundPathprocessorInterface does not. Let's fix that.

Proposed resolution

Update the docblock on OutboundPathProcessorInterface that $path must have a leading /. See InboundPathProcessorInterface for an example.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the API is unclear.
Issue priority Normal because nothing is seriously broken, just slightly broken.
Disruption This is no longer an API change in practice, and even if something is affected it's already wrong if it's not using a leading-/ path so the bug to fix would be there anyway. Just a docfix. Move along. :-)

Comments

catch’s picture

Yes this turned me around several times on the route caching patch.

Crell’s picture

Category: Task » Bug report
Status: Active » Postponed

I'm going to take #2 as endorsement to move forward here. :-) To avoid unnecessary rerolls, marking it postponed on the original issue; once that's in this should be a pretty simple patch.

Also fixing status to align with the beta summary; I probably fat-fingered when filing.

catch’s picture

StatusFileSize
new42.73 KB

I think you're underestimating how broken this is. "We've been trying to standardize on leading-/ for paths" was never codified anywhere or applied consistently.

The thing that turned me around on the other patch was that path alias storage stores without a leading slash.

Additionally AliasManager expects a path with no leading slash.

So to get rid of unnecessary trimming and adding of '/', this will need to be standardized. Not just the path processors themselves.

Then I did a quick bit of testing of the path alias storage vs. menu UIi and found #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals. Opened that as a separate critical bug since it's a data-integrity issue, but it might need to be fixed at the same time as this.

Crell’s picture

Original blocker is in, but leaving as postponed on #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals for now.

dawehner’s picture

Status: Postponed » Active

Back to active.

Crell’s picture

Issue summary: View changes
Issue tags: +Novice, +Documentation

The previous issues have fixed up all the bugs here, it looks like, save for documentation. InboundPathProcessorInterface now specifies leading-/, but outbound does not. Therefore, turning this into a novice docfix issue.

hussainweb’s picture

Status: Active » Needs review
StatusFileSize
new1.64 KB

Getting this started. There are some other small changes in the docblock as well.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Hey, you're not a novice! :-)

hussainweb’s picture

I sure am. Every time I get back to issue queues, everything is very different. :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: clarify_pathprocessor-2508037-8.patch, failed testing.

Status: Needs work » Needs review
hussainweb’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per #10.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: clarify_pathprocessor-2508037-8.patch, failed testing.

Status: Needs work » Needs review
hussainweb’s picture

Status: Needs review » Reviewed & tested by the community

Resetting to RTBC as per #10 after random failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: clarify_pathprocessor-2508037-8.patch, failed testing.

Status: Needs work » Needs review
hussainweb’s picture

Once again, Resetting to RTBC as per #10 after random failure.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Pay attention, Drupal.org!

webchick’s picture

Component: routing system » documentation
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 32661b4 on 8.0.x
    Issue #2508037 by hussainweb, catch, Crell: Clarify PathProcessor path...

Status: Fixed » Closed (fixed)

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