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

AdminBarRemoval: More thorough checking for admin bar removal and hiding #817

Merged
merged 5 commits into from
Feb 1, 2017

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jan 23, 2017

This sniff has been completely overhauled with the additional wish-list of the Theme Review Team in mind.

Originally the sniff only checked for show_admin_bar( ... ) and add_filter( 'show_admin_bar', ... ).

  • It didn't contain a further check for the actual value passed to these functions.
  • It didn't check for usage of CSS to hide the admin bar

This PR fixes that.
Includes extensive additional unit tests.

Notes:

  • The sniff now checks both PHP as well as CSS files.
  • The class now extends the WordPress_AbstractFunctionParameterSniff for the PHP function parameter checks.
  • It overloads the register() method to register additional tokens.
  • It overloads the process() method of that class to only use the functions in the abstract class when passed a T_STRING token. For the other token types, the processing is handled within the sniff.
  • The sniff now contains a public property $remove_only which defaults to true, to toggle whether to just check that the admin bar is being manipulated or to do a more in-depth check and only throw errors when removing or hiding of the admin bar is detected.
  • The sniff also contains a new utility function to work around a bug in the PHP 5.2 tokenizer regarding text strings containing <s, like <style>.

Who should I ping from the VIP team to have a look as well ?

jrfnl added 2 commits January 23, 2017 17:21

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…checking the function calls.

Adds a `$remove_only` property to toggle whether to disallow admin bar removal or disallow any manipulation of the visibility of the admin bar.
@grappler
Copy link
Member

Please can you have a look @sboisvert @david-binda

*
* {@internal For PHP 5.3+ this is straightforward, just check if $content
* contains the tag.
* PHP 5.2 however, creates a seperate token for `<s` when used in inline HTML,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: seperate => separate`

Why did the tokenizer create a separate token in PHP 5.2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did the tokenizer create a separate token in PHP 5.2?

See https://bugs.php.net/bug.php?id=48446:

It stops at "<s" to avoid a long PHP opening tag (e.g. <script language="php">,
etc.) from being taken as inline HTML.

The good news, however, is that a new scanner is used for PHP 5.3, and as of a few weeks ago (5.3.0 RC2), it now works as you'd expect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S.: Fixed the typo and added the link to the documentation.

@jrfnl jrfnl force-pushed the feature/stricter-adminbar-removal-check branch from 42502ed to 6c7b05a Compare January 24, 2017 07:13
@sboisvert
Copy link
Contributor

I haven't read the code in detail but just the goal of what this is achieving is good from VIP's point of view, thanks!

@jrfnl
Copy link
Member Author

jrfnl commented Jan 24, 2017

@sboisvert Thanks for looking through it.

@GaryJones GaryJones merged commit 936b17d into develop Feb 1, 2017
@GaryJones GaryJones deleted the feature/stricter-adminbar-removal-check branch February 1, 2017 07:51
@jrfnl jrfnl added this to the 0.11.0 milestone Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants