Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New sniff] No disabling of the admin toolbar. #1

Closed
2 tasks
jrfnl opened this issue Jul 10, 2016 · 5 comments
Closed
2 tasks

[New sniff] No disabling of the admin toolbar. #1

jrfnl opened this issue Jul 10, 2016 · 5 comments

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Jul 10, 2016

Rule:

No hiding of the admin bar - check if show_admin_bar( false ) is called or if add_filter( 'show_admin_bar', '__return_false' ) is somewhere in the code.

Ref: https://make.wordpress.org/themes/handbook/review/required/#core-functionality-and-features

Theme check file covering this rule:

https://github.com/Otto42/theme-check/blob/master/checks/adminbar.php

To do:

- [ ] Create unit tests
- [ ] Create new sniff

  • Open PR upstream in WPCS to improve the admin bar removal sniff.
  • Add WordPress_Sniffs_VIP_AdminBarRemovalSniff sniff to the ruleset.
@jrfnl
Copy link
Contributor Author

jrfnl commented Jul 11, 2016

By the looks of it, there already is a sniff for this in WP-VIP: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/VIP/AdminBarRemovalSniff.php

The check in Theme Check however is more comprehensive, so I suggest improving the VIP check upstream and then adding it to the Theme rule set.

@grappler
Copy link
Member

@jrfnl Was there something specific you saw that needed improving other then add a check for #wpadmin and .show-admin-bar?

@jrfnl
Copy link
Contributor Author

jrfnl commented Jul 17, 2016

@grappler I believe those checks should be done on both the PHP as well as CSS files and should be quite specific. I mean, a theme changing the color of the admin bar to be in line with their front-end theme should not see this notice.

I also think that for the existing PHP function based check, the parameter values should be checked against some whitelisted values.
I don't think: add_filter( 'show_admin_bar', '__return_true' ); should trigger this error and while it will be very rare indeed to come across that code, it shouldn't be hard to check for.

I also think the sniff should be moved from Sniffs/VIP/ to Sniffs/WP/ as it's not VIP specific, though we will need to check if VIP agrees with the proposed adjustments and if not, have a second sniff just for the CSS related checks.

I have made a small start locally for the changes needed upstream - mainly creating the unit tests. Want me to share this for you to continue ?

@justintadlock
Copy link

I believe those checks should be done on both the PHP as well as CSS files and should be quite specific. I mean, a theme changing the color of the admin bar to be in line with their front-end theme should not see this notice.

Definitely agree. I've said as much and thought that we had decided not to target CSS a while back. I think we'd all be OK with a check for something like this (or variations):

#wpadminbar { display: none; }

I also think that for the existing PHP function based check, the parameter values should be checked against some whitelisted values.
I don't think: add_filter( 'show_admin_bar', '__return_true' ); should trigger this error and while it will be very rare indeed to come across that code, it shouldn't be hard to check for.

Force-showing is as equally bad as force hiding it. Whether to show/hide the admin bar is outside the scope of themes. This is core and plugin territory.

@jrfnl
Copy link
Contributor Author

jrfnl commented Jul 17, 2016

Whether to show/hide the admin bar is outside the scope of themes. This is core and plugin territory.

The altered sniff will need to be as re-usable as possible as it is used for plugins as well in VIP context. So in that case, we either need a property switch which allows for checking that those functions are used at all (themes) or checking that those functions don't hide the admin bar (rest of the world).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants