Problem/Motivation

If the user does not have access "Use Admin Toolbar search", the Search link is still added but without styles and looks broken.

Proposed resolution

Check user access before displaying the Search link in the admin toolbar.

Similar to #3115535 and #3117860.

CommentFileSizeAuthor
#14 3126311-check-permissions-for-search-access-14.patch649 bytesyahyaalhamad
#8 3126311-check-permissions-for-search-access-8.patch4.39 KBsasanikolic
#6 3126311-check-permissions-for-search-access-6-interdiff.txt6.52 KBsasanikolic
#6 3126311-check-permissions-for-search-access-6.patch4.59 KBsasanikolic
#3 3126311-check-permissions-for-search-access.patch2.56 KBsasanikolic
#2 Broken admin toolbar search.png22.61 KBsasanikolic

Comments

sasanikolic created an issue. See original summary.

sasanikolic’s picture

sasanikolic commented
StatusFileSize
new Broken admin toolbar search.png22.61 KB

Forgot to attach the screenshot.

sasanikolic’s picture

sasanikolic commented
Status: Active » Needs review
StatusFileSize
new 3126311-check-permissions-for-search-access.patch2.56 KB

Here is the patch with the improved check for access. Seems like the #access didn't work on the toolbar_item element. We need to wrap it in an if in order to not display anything (like libraries).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I'd suggest an early return if you don't have access instead, that will be a much easier patch to review.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Also, seems like it should be possible to test this?

sasanikolic’s picture

sasanikolic commented
StatusFileSize
new 3126311-check-permissions-for-search-access-6.patch4.59 KB
new 3126311-check-permissions-for-search-access-6-interdiff.txt6.52 KB

I added the check and return and improved the tests as suggested above.

adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented
Status: Needs review » Needs work

Thanks, @Berdir and @sasanikolic but the patch is now outdated after commit #3122957: Show search field by default? I running low of time now, can you reroll the patch?

sasanikolic’s picture

sasanikolic commented
Status: Needs work » Needs review
StatusFileSize
new 3126311-check-permissions-for-search-access-8.patch4.39 KB

Sure, I rebased the patch now.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good now I think.

  • adriancid committed 6dec556 on 8.x-2.x authored by sasanikolic 
    Issue #3126311 by sasanikolic, Berdir, adriancid: Check permissions...

  • adriancid committed f3620de on 8.x-2.x authored by sasanikolic 
    Issue #3126311 by sasanikolic, Berdir, adriancid: Check permissions...
adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Thanks

Status: Fixed » Closed (fixed)

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

yahyaalhamad’s picture

yahyaalhamad
Location Palestine
commented
StatusFileSize
new 3126311-check-permissions-for-search-access-14.patch649 bytes

Uploaded a fast fix patch for 2.2 until a new version is released.