No Favicon #78

khacoder · 2016-08-29T04:47:34Z

First Commit
Issue #70

grappler · 2016-09-18T14:33:55Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+		);
+		foreach ( $checks as $check ) {
+			if ( false !== strpos( $token['content'], $check ) ) {
+				$phpcsFile->addError( 'Possible Favicon found. Favicons are handled by the Site Icon setting in the customizer since version 4.3.' , $stackPtr, 'NoFavicon' );


What about "Code for Favicons found."?

jrfnl · 2016-10-11T16:23:59Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+	 */
+	public function register() {
+		return array(
+			T_STRING,


Why are you sniffing T_STRING ?

jrfnl · 2016-10-11T16:25:19Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+	public function register() {
+		return array(
+			T_STRING,
+			T_CONSTANT_ENCAPSED_STRING,


Better to use the build in PHP_CodeSniffer_Tokens::$stringTokens and add T_INLINE_HTML and any other tokens you really need, than hard-code T_CONSTANT_ENCAPSED_STRING.

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

jrfnl · 2016-10-11T16:26:00Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+ * @category PHP
+ * @package  PHP_CodeSniffer
+ * @link     https://make.wordpress.org/core/handbook/best-practices/coding-standards/
+ */


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

jrfnl · 2016-10-11T16:26:08Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+ * @category  PHP
+ * @package   PHP_CodeSniffer
+ * @author    khacoder
+ */


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

jrfnl · 2016-10-11T16:27:00Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+		$tokens = $phpcsFile->getTokens();
+		$token  = $tokens[ $stackPtr ];
+
+		$checks = array(


This list should be a class property.

jrfnl · 2016-10-11T16:32:29Z

WordPress/Tests/Theme/NoFaviconUnitTest.inc

@@ -0,0 +1,6 @@
+/* ----- Bad ------- ERROR ---- */
+<link rel="shortcut icon" href="<?php myfunction('link' ); ?>">


Please make the test files lead by example and not include other bad practices, the href functions should all have esc_url()s around them. Alternatively just hardcode a fake url.

jrfnl · 2016-10-11T16:32:46Z

WordPress/Tests/Theme/NoFaviconUnitTest.php

+ * @category PHP
+ * @package  PHP_CodeSniffer
+ * @link     https://make.wordpress.org/core/handbook/best-practices/coding-standards/
+ */


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

jrfnl · 2016-10-11T16:32:50Z

WordPress/Tests/Theme/NoFaviconUnitTest.php

+ * @category PHP
+ * @package  PHP_CodeSniffer
+ * @author   khacoder
+ */


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

jrfnl · 2016-10-11T16:32:55Z

WordPress/Tests/Theme/NoFaviconUnitTest.php

+	 * The key of the array should represent the line number and the value
+	 * should represent the number of errors that should occur on that line.
+	 *
+	 * @return array(int => int)


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

jrfnl · 2016-10-11T16:33:00Z

WordPress/Tests/Theme/NoFaviconUnitTest.php

+	 * The key of the array should represent the line number and the value
+	 * should represent the number of warnings that should occur on that line.
+	 *
+	 * @return array(int => int)


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

jrfnl · 2016-10-14T02:58:20Z

Unit tests are failing, but I think I can see why - will leave the solution inline.

P.S.: I did say it was pseudo-code with the code example, so untested, untried....

jrfnl · 2016-10-14T03:03:20Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+class WordPress_Sniffs_Theme_NoFaviconSniff implements PHP_CodeSniffer_Sniff {
+
+	const REGEX_TEMPLATE = '` (%s)`i';
+	const REGEX_ATTR_TEMPLATE = '%1$s=[\'"]%2$s[\'"]';


Replace these two lines like so and I believe the unit tests will pass (fingers crossed - still doing this without running actual tests 😎 ):

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

Change in the first line isn't really needed, but can't hurt either.

That's why we have unit tests. 😎 How would you write the inline docs for these constants.

Similar to how you document the class properties. I'm not sure about the tag to use at the moment - I believe at some point there was a @const tag, but would need to check if that's still a thing.
To check you could have a look through the current version of the PSR proposal for documentation and at the PHPDocumentor documentation.

jrfnl · 2016-10-14T03:05:08Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+	public function register() {
+		$regex_parts = array();
+		foreach( $this->attribute_blacklist as $key => $values ) {
+			$values = array_map( 'preg_quote', $values );


Oh and to be extremely precise, we should pass the delimiter to preg_quote, though it's not going to make any difference in this case:

$values = array_map( 'preg_quote', $values, array_fill ( 0, count( $values ), '`' ) );

jrfnl · 2016-10-14T03:09:44Z

Unit tests still failing, but I can see why - we're matching on the whole attribute attr="value" and some of the values for rel at the moment are partial values, so they won't match.

jrfnl · 2016-10-14T03:15:28Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+			'icon',
+			'apple-touch-icon',
+			'apple-touch-icon-precomposed',
+		),


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__ ) ), '" />';

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.

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

First Commit Issue #70

grappler · 2016-10-17T02:04:57Z

The checks are passing now.

jrfnl · 2016-10-22T16:47:38Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+	 * @return array
+	 */
+	public function register() {
+		$regex_parts = array();


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

jrfnl · 2016-10-22T16:49:28Z

WordPress/Sniffs/Theme/NoFaviconSniff.php

+		$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' );


The capitalization in the error sentence seems a bit strange.
What about :
'Code for favicon found. Favicons are handled by the "Site Icon" setting in the customizer since version 4.3.'

jrfnl · 2016-10-22T16:51:42Z

WordPress/Tests/Theme/NoFaviconUnitTest.inc

+<?php
+echo '
+<meta name="msapplication-config" content="/my/path/browserconfig.xml" />
+<meta name="msapplication-TileImage" content="', esc_url( plugins_url( 'images/icons/mstile-144x144.png?v=' . self::VERSION, __FILE__ ) ), '" />


I realize I provided the example code, but it came from a plugin... as this is a theme check sniff, using plugins_url() in the example code feels misleading to me.

jrfnl · 2016-10-22T16:53:30Z

Great. Three more minor comments to dot the i's...

grappler reviewed Sep 18, 2016

View reviewed changes

grappler added the pr: needs review label Sep 27, 2016

grappler force-pushed the feature/theme-review-sniffs branch 2 times, most recently from 563cd1d to 6671823 Compare September 27, 2016 12:28

jrfnl requested changes Oct 11, 2016

View reviewed changes

grappler self-assigned this Oct 12, 2016

jrfnl approved these changes Oct 14, 2016

View reviewed changes

jrfnl reviewed Oct 14, 2016

View reviewed changes

khacoder and others added 3 commits October 17, 2016 10:59

No Favicon

2b3c2dd

First Commit Issue #70

Update inline docs

9738e77

Update inline docs & improve the check

6f644c5

jrfnl reviewed Oct 22, 2016

View reviewed changes

jrfnl added pr: needs update and removed pr: needs review labels Oct 22, 2016

Doting the i’s

a167a7f

grappler added pr: needs review and removed pr: needs update labels Oct 23, 2016

jrfnl merged commit 6cbc3d9 into WPTT:feature/theme-review-sniffs Oct 24, 2016

jrfnl removed the pr: needs review label Oct 27, 2016

grappler mentioned this pull request Dec 18, 2016

Add sniffs for the theme checks WordPress/WordPress-Coding-Standards#578

Closed

72 tasks

jrfnl added this to the 0.1.0 milestone Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No Favicon #78

No Favicon #78

khacoder commented Aug 29, 2016

grappler Sep 18, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl commented Oct 14, 2016

jrfnl Oct 14, 2016

grappler Oct 14, 2016

jrfnl Oct 14, 2016 •

edited

Loading

jrfnl Oct 14, 2016

jrfnl commented Oct 14, 2016 •

edited

Loading

jrfnl Oct 14, 2016 •

edited

Loading

grappler Oct 14, 2016

jrfnl Oct 14, 2016

grappler commented Oct 17, 2016

jrfnl Oct 22, 2016 •

edited

Loading

jrfnl Oct 22, 2016 •

edited

Loading

jrfnl Oct 22, 2016

jrfnl commented Oct 22, 2016

		@@ -0,0 +1,6 @@
		/* ----- Bad ------- ERROR ---- */
		<link rel="shortcut icon" href="<?php myfunction('link' ); ?>">

No Favicon #78

No Favicon #78

Conversation

khacoder commented Aug 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Oct 14, 2016 • edited Loading

jrfnl Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grappler commented Oct 17, 2016

jrfnl Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

jrfnl Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Oct 22, 2016

jrfnl Oct 14, 2016 •

edited

Loading

jrfnl commented Oct 14, 2016 •

edited

Loading

jrfnl Oct 14, 2016 •

edited

Loading

jrfnl Oct 22, 2016 •

edited

Loading

jrfnl Oct 22, 2016 •

edited

Loading