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.

Sign up for GitHub

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

No Favicon #78

Merged
merged 4 commits into from
Oct 24, 2016
Merged

No Favicon #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b3c2dd
No Favicon
Aug 29, 2016
9738e77
Update inline docs
grappler Oct 13, 2016
6f644c5
Update inline docs & improve the check
grappler Oct 17, 2016
a167a7f
Doting the i’s
grappler Oct 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions WordPress/Sniffs/Theme/NoFaviconSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
<?php
/**
* WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
* @license https://opensource.org/licenses/MIT MIT
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the doc block to comply with the current standards.


/**
* Check for hardcoded favicons instead of using core implementation.
*
* @link https://make.wordpress.org/themes/handbook/review/required/#core-functionality-and-features
*
* @package WPCS\WordPressCodingStandards
*
* @since 0.xx.0
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the doc block to comply with the current standards.

class WordPress_Sniffs_Theme_NoFaviconSniff implements PHP_CodeSniffer_Sniff {

const REGEX_TEMPLATE = '` (?:%s)`i';

const REGEX_ATTR_TEMPLATE = '%1$s=[\'"](?:%2$s)[\'"]';

/**
* List of link and meta attributes that are blacklisted.
*
* @var array
*/
protected $attribute_blacklist = array(
'rel' => array(
'icon',
'shortcut icon',
'bookmark icon',
'apple-touch-icon',
'apple-touch-icon-precomposed',
),
Copy link
Contributor

@jrfnl jrfnl Oct 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

We should add:

  • 'shortcut icon'
  • 'bookmark icon'

Not sure about 'icon shortcut' as I'm not sure that's even a valid value. If it is, it should be added to the list as well.

These could also be considered for addition (both in unit tests as well as in the list):

<meta name="apple-mobile-web-app-title" content="<?php echo self::APP_NAME; ?>" />
<?php
echo '
<meta name="msapplication-config" content="/my/path/browserconfig.xml" />
<meta name="msapplication-TileColor" content="#d6001e" />
<meta name="msapplication-TileImage" content="', esc_url( plugins_url( 'images/icons/mstile-144x144.png?v=' . self::VERSION, __FILE__ ) ), '" />
<meta name="msapplication-square70x70logo" content="', esc_url( plugins_url( 'images/icons/tiny.png?v=' . self::VERSION, __FILE__ ) ), '" />
<meta name="msapplication-square150x150logo" content="', esc_url( plugins_url( 'images/icons/square.png?v=' . self::VERSION, __FILE__ ) ), '" />
<meta name="msapplication-wide310x150logo" content="', esc_url( plugins_url( 'images/icons/wide.png?v=' . self::VERSION, __FILE__ ) ), '" />
<meta name="msapplication-square310x310logo" content="', esc_url( plugins_url( 'images/icons/large.png?v=' . self::VERSION, __FILE__ ) ), '" />';

Copy link
Member

Choose a reason for hiding this comment

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

The previous best practice was to have 'shortcut icon'. I don't think the reverse was ever valid. https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types

I wonder what the benafits of adding extra checks would be. I expect most implementations of the favicon would all or nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that they probably wouldn't catch any more, but the benefit is of course completeness.

'name' => array(
'msapplication-config',
'msapplication-TileImage',
'msapplication-square70x70logo',
'msapplication-square150x150logo',
'msapplication-wide310x150logo',
'msapplication-square310x310logo',
),
);

/**
* The regex to catch the blacklisted attributes.
*
* @var string
*/
protected $favicon_regex;

/**
* Returns an array of tokens this test wants to listen for.
*
* @return array
*/
public function register() {
// Build the regex to be used only once.
$regex_parts = array();
Copy link
Contributor

@jrfnl jrfnl Oct 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe add a comment above this line // Build the regex to be used only once. ?


foreach ( $this->attribute_blacklist as $key => $values ) {
$values = array_map( 'preg_quote', $values, array_fill( 0, count( $values ), '`' ) );
$values = implode( '|', $values );
$regex_parts[] = sprintf( self::REGEX_ATTR_TEMPLATE, preg_quote( $key ), $values );
}

$this->favicon_regex = sprintf( self::REGEX_TEMPLATE, implode( '|', $regex_parts ) );

$tokens = PHP_CodeSniffer_Tokens::$stringTokens;
$tokens[] = T_INLINE_HTML;

return $tokens;
}

/**
* Processes this test, when one of its tokens is encountered.
*
* @param PHP_CodeSniffer_File $phpcsFile The file being scanned.
* @param int $stackPtr The position of the current token
* in the stack passed in $tokens.
*
* @return void
*/
public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
$tokens = $phpcsFile->getTokens();
$token = $tokens[ $stackPtr ];

if ( preg_match( $this->favicon_regex, $token['content'] ) > 0 ) {
$phpcsFile->addError( 'Code for favicon found. Favicons are handled by the "Site Icon" setting in the customizer since version 4.3.' , $stackPtr, 'NoFavicon' );
}

}

}
21 changes: 21 additions & 0 deletions WordPress/Tests/Theme/NoFaviconUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@

<html <?php language_attributes(); ?> class="no-js"> // OK.
<meta charset="<?php bloginfo( 'charset' ); ?>"> // OK.
<meta name="viewport" content="width=device-width, initial-scale=1"> // OK.
<link rel="profile" href="http://gmpg.org/xfn/11"> // OK.


<link rel="shortcut icon" href="http://example.com/favicon.ico"> // Error.
<link rel="icon" href="<?php esc_url( favicon_url( 'link' ) ); ?>"> // Error.
<link rel='apple-touch-icon' href="<?php esc_url( favicon_url( 'link' ) ); ?>"> // Error.
<link href="<?php esc_url( favicon_url( 'link' ) ); ?>" rel="apple-touch-icon-precomposed"> // Error.
<meta name="msapplication-TileImage" href="<?php esc_url( favicon_url( 'link' ) ); ?>"> // Error.

<?php
echo '
<meta name="msapplication-config" content="/my/path/browserconfig.xml" />
<meta name="msapplication-TileImage" content="', esc_url( get_template_directory() . 'images/icons/mstile-144x144.png?v=' . self::VERSION ), '" />
<meta name="msapplication-square70x70logo" content="', esc_url( get_template_directory() . 'images/icons/tiny.png?v=' . self::VERSION ), '" />
<meta name="msapplication-square150x150logo" content="', esc_url( get_template_directory() . 'images/icons/square.png?v=' . self::VERSION ), '" />
<meta name="msapplication-wide310x150logo" content="', esc_url( get_template_directory() . 'images/icons/wide.png?v=' . self::VERSION, __FILE__ ) ), '" />
<meta name="msapplication-square310x310logo" content="', esc_url( get_template_directory() . 'images/icons/large.png?v=' . self::VERSION ), '" />';
48 changes: 48 additions & 0 deletions WordPress/Tests/Theme/NoFaviconUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php
/**
* Unit test class for WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
* @license https://opensource.org/licenses/MIT MIT
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the doc block to comply with the current standards.


/**
* Unit test class for the NoFavicon sniff.
*
* @package WPCS\WordPressCodingStandards
* @since 0.xx.0
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the doc block to comply with the current standards.

class WordPress_Tests_Theme_NoFaviconUnitTest extends AbstractSniffUnitTest {

/**
* Returns the lines where errors should occur.
*
* @return array <int line number> => <int number of errors>
*/
public function getErrorList() {
return array(
8 => 1,
9 => 1,
10 => 1,
11 => 1,
12 => 1,
16 => 1,
17 => 1,
18 => 1,
19 => 1,
20 => 1,
21 => 1,
);
}

/**
* Returns the lines where warnings should occur.
*
* @return array <int line number> => <int number of warnings>
*/
public function getWarningList() {
return array();
}

} // End class.