Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#32221 closed defect (bug) (fixed)

Custom nav menu items are scheme-sensitive

Reported by: dd32's profile dd32 Owned by: dd32's profile dd32
Milestone: 4.3 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: good-first-bug has-patch needs-testing dev-feedback
Focuses: Cc:

Description

If you have a website that's available over both https and http, adding a custom URL entry as HTTP will cause it not to be marked as current-menu-item when visited over HTTPS (and vice versa)

At present, custom nav menu's are only marked as present when one of the following is valid:

  1. Exact match (http://example.com/page/
  2. Match without trailing slash ( http://example.com/page
  3. Match with path only ( /page/ )

It seems to me, that both the first and second instances should apply set_url_scheme() to both forms it's comparing (or use some other comparison method that ignores the protocol).

Attachments (2)

32221.patch (1.2 KB) - added by McGuive7 9 years ago.
Fixes to more accurately add current-menu-item class for custom menu items when URLs aren't an exact match (https/http, URL parameters)
32221.2.patch (980 bytes) - added by marsjaninzmarsa 9 years ago.

Download all attachments as: .zip

Change History (15)

#1 @McGuive7
9 years ago

I also noticed that the current-menu-item check fails for custom menu links if any parameters are appended to the URL (e.g. http://mysite.com/?foo=bar)

I can't imagine a case in which this would be desirable - am I missing something?

@McGuive7
9 years ago

Fixes to more accurately add current-menu-item class for custom menu items when URLs aren't an exact match (https/http, URL parameters)

#2 @McGuive7
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Version set to trunk

This patch adds support for the following issue mentioned above:

  1. HTTPS vs. HTTP - apply set_url_scheme() when retrieving $item_url
  2. URL parameters - apply strtok() to only compare everything before any ? in the URL

#3 @marsjaninzmarsa
9 years ago

Duh, you was first. :D

#4 @DrewAPicture
9 years ago

  • Owner set to McGuive7
  • Status changed from new to assigned

#5 @marsjaninzmarsa
9 years ago

What about not http/https patchs, but relative (point 3)? Did your patch, @McGuive7, covers it? Is this part of core unit-tested?

#6 @McGuive7
9 years ago

If you re-read the initial message, those are actually 3 scenarios that do successfully work - so relative paths do work already (at least in the latest version of trunk).

#7 @marsjaninzmarsa
9 years ago

I've just been worried about set_url_scheme() will be destructive to relative urls, but after check - you are right, your patch is definitely better than mine. :)

#8 @McGuive7
9 years ago

Cool, so now what happens? What's the process for getting this reviewed and integrated? Anyone?

#9 @johnbillion
9 years ago

  • Milestone changed from Future Release to 4.3
  • Version changed from trunk to 3.0

Related: #32367

#10 @McGuive7
9 years ago

  • Keywords dev-feedback added

#11 @obenland
9 years ago

@dd32, can you take a look at the patch?

#12 @dd32
9 years ago

  • Owner changed from McGuive7 to dd32
  • Status changed from assigned to accepted

I also noticed that the current-menu-item check fails for custom menu links if any parameters are appended to the URL (e.g. http://mysite.com/?foo=bar)

I think this is deliberate.

Take for example the following url's:

All of those should be able to be in a menu and not highlight the others items.

I can understand that however you'd want the following to highlight:

However, what's to say that those have the same content? maybe they should be different menu items too?

I guess it's those hard questions which lead to that case not being handled, but there are filters in place to allow that sort of thing..

For that reason 32221.2.patch by @marsjaninzmarsa looks like it's the best way to go here, but I'll do some testing and get something in soon.

#13 @dd32
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 33103:

Ensure that Custom Links in menu's are marked as current regardless of their scheme.
Props McGuive7, marsjaninzmarsa.
Fixes #32221

Note: See TracTickets for help on using tickets.