WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#42991 closed defect (bug) (fixed)

Customizer: Search Menu item not working on Custom Link

Reported by: monikarao Owned by: westonruter
Milestone: 4.9.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit fixed-major
Focuses: ui Cc:

Description

In Customizer "Search Menu Item Field" is present to search all menu items but it's not working on Custom Links. Ex. Added "Home" as Custom Link but when searching this "Home" it displayed message "no-result-found".

Attachments (9)

Home-Custom-Link.png (10.1 KB) - added by monikarao 5 months ago.
Created Home Custom Link
No-Result-Found.png (3.0 KB) - added by monikarao 5 months ago.
On Searching "Home" - No Result Found Message
42991.diff (711 bytes) - added by audrasjb 4 months ago.
Add home custom link in customizer menu search
42991-home-link-in-customizer-search.png (62.7 KB) - added by audrasjb 4 months ago.
Result
42991.2.diff (905 bytes) - added by audrasjb 4 months ago.
add if statement before Home item appending
9b3c07da700f76ddf897a62994773d14.gif (126.7 KB) - added by audrasjb 4 months ago.
Test: not appending on "Test" search but in "Hom" string yes :)
42991.3.diff (1.2 KB) - added by audrasjb 4 months ago.
Add mb_stripos/stripos function and Yoda styled conditions
42991.4.diff (1.2 KB) - added by audrasjb 4 months ago.
Remove extra spaces L. 391 & 402
42991.5.diff (971 bytes) - added by audrasjb 4 months ago.
Here is a new patch without code duplication

Download all attachments as: .zip

Change History (35)

@monikarao
5 months ago

Created Home Custom Link

@monikarao
5 months ago

On Searching "Home" - No Result Found Message

#1 @melchoyce
5 months ago

  • Milestone changed from Awaiting Review to 4.9.2

Good catch!

#2 @monikarao
5 months ago

Thanks @melchoyce

@audrasjb
4 months ago

Add home custom link in customizer menu search

#3 @audrasjb
4 months ago

  • Keywords has-patch added

Hi @monikarao @melchoyce

I made a patch for this ticket. It works well. Here is a screenshot of the result.

Cheers, Jb

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


4 months ago

#5 @westonruter
4 months ago

  • Keywords needs-testing added
  • Owner set to melchoyce
  • Status changed from new to assigned

#6 @dd32
4 months ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 months ago

#8 @Clorith
4 months ago

  • Keywords has-patch needs-testing removed

Thanks for the patch, looking over it I notice it's currently appending the Home menu item to any search.

It will need an if statement there to make sure the translatable Home label matches before being appended

@audrasjb
4 months ago

add if statement before Home item appending

@audrasjb
4 months ago

Test: not appending on "Test" search but in "Hom" string yes :)

#9 @audrasjb
4 months ago

  • Keywords has-patch added

Thanks @Clorith for the review, you're right !

In 42991.2.diff I added a condition to check if the entered string matches with Home (translatable).

Last edited 4 months ago by audrasjb (previous) (diff)

#10 @westonruter
4 months ago

@audrasjb tip: you can use stripos() instead of strpos() with multiple strtolower() calls.

#11 @westonruter
4 months ago

  • Keywords needs-patch needs-testing added; has-patch removed

On second thought, this perhaps should use mb_stripos() if possible since the translation string will have multi-byte characters in it. For example compare:

wp> stripos( 'PAPÁ', 'á' )
=> bool(false)
wp> mb_stripos( 'PAPÁ', 'á' )
=> int(3)

I believe you'll have to check if the mb_stripos() function exists since the multibyte string functions may not be present, and in such case you'll need to use stripos(). At least, _wp_json_convert_string() checks for the existence of an mb_* function prior to using it.

#12 @audrasjb
4 months ago

Thanks @westonruter for your help, this is very interesting I'm really happy to learn things at the same time I contribute :)

So, here is a new tested patch which seems to work fine. Is this better?

@audrasjb
4 months ago

Add mb_stripos/stripos function and Yoda styled conditions

#13 @audrasjb
4 months ago

  • Keywords has-patch added; needs-patch removed

@audrasjb
4 months ago

Remove extra spaces L. 391 & 402

#14 @westonruter
4 months ago

@audrasjb I suggest removing some of the duplication, following something like this:

<?php
if ( isset( $args['s'] ) ) {
        $title = _x( 'Home', 'nav menu home label' );
        $matches = function_exists( 'mb_stripos' ) ? false !== mb_stripos( $title, $args['s'] ) : false !== stripos( $title, $args['s'] );
        if ( $matches ) {
                $items[] = array(
                        // ...
                        'title'      => $title,
                        // ...
                );
        }
}

#15 @audrasjb
4 months ago

Thanks @westonruter for the suggestion and for this hint. I was wondering if I had to duplicate or not… Let's roll! Will send a new version soon.

@audrasjb
4 months ago

Here is a new patch without code duplication

#16 @westonruter
4 months ago

@melchoyce does 42991.5.diff work as you expect?

#17 @melchoyce
4 months ago

Looks good to me!

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


4 months ago

#19 @westonruter
4 months ago

  • Keywords commit added; needs-testing removed
  • Owner changed from melchoyce to westonruter
  • Status changed from assigned to accepted

#20 @westonruter
4 months ago

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

In 42611:

Customize: Include nav menu item for Home custom link in search results for "Home".

Props audrasjb, westonruter.
Fixes #42991.

#21 @westonruter
4 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from trunk to 4.3

#22 @SergeyBiryukov
4 months ago

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

In 42621:

Customize: Include nav menu item for Home custom link in search results for "Home".

Props audrasjb, westonruter.
Merges [42611] to the 4.9 branch.
Fixes #42991.

#23 @monikarao
4 months ago

Hello @SergeyBiryukov

I didn't get props on this ticket.

#24 @SergeyBiryukov
4 months ago

Hi @monikarao,

In the past only the patch author would generally get the props, however recently we've indeed started to give props for other contributions as well, including bug reports. I'll make sure to include you in the contributors list for 4.9.3. Thanks for the ticket!

#25 @monikarao
4 months ago

Thanks @SergeyBiryukov

This ticket was mentioned in Slack in #core by chetan200891. View the logs.


4 months ago

Note: See TracTickets for help on using tickets.