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

Add WordPress_AbstractThemeSniff Class #53

Closed
wants to merge 7 commits into from
Closed

Add WordPress_AbstractThemeSniff Class #53

wants to merge 7 commits into from

Conversation

khacoder
Copy link
Contributor

As per issue #45

khacoder added 4 commits July 31, 2016 18:43
- Change to abstract class
- Change file name to AbstractThemeSniff.php
- remove themesniff.php
- change class name to WordPress_AbstractThemeSniff
- completely overhaul code
$sniff_helper is created.
Once through checks ported from themecheck.
@@ -0,0 +1,1068 @@
<?php

Choose a reason for hiding this comment

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

File header documentation exists twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it doesn't. One is the File doc block, the other the class doc block.

FYI: in the main WPCS a clean up has taken place of all doc block documentation and the TRTCS sniffs should start complying with the latest standards used in WPCS once the latest WPCS has been merged into TRTCS (and a clean up should be done of already merged sniffs).

@Otto42
Copy link
Member

Otto42 commented Aug 28, 2016

We cannot use any code from the Theme Check as it is released under the GPL licence.

I wrote that code. You have my explicit permission to use any and all code from theme-check regardless of licensing.

@jrfnl
Copy link
Contributor

jrfnl commented Aug 28, 2016

I wrote that code. You have my explicit permission to use any and all code from theme-check regardless of licensing.

Unfortunately, permission apparrently is not enough. It's the licensing that's important.
See a similar discussion we recently had in WPCS: WordPress/WordPress-Coding-Standards#608

@Otto42
Copy link
Member

Otto42 commented Aug 29, 2016

Permission absolutely is enough. If I wrote that code, and I will check if you tell me what to look for, then I'm the only person who could possibly object to its use.

I categorically do not object to such use. We plan on using this code on .org eventually. Use whatever you need.

 - blacklist_file_check()
 - get_textdomains()
 - get_themeslug()
Minor changes
- added check for $files[0] in __construct()
- other minor bug fixes
if ( false !== strpos( $file_content, 'register_nav_menu' ) ) {
$sniff_helper['theme_supports']['custom-menu'] = true;
}
if ( false !== strpos( $file_content, 'wp_nav_menu' ) ) {
Copy link
Member

@grappler grappler Sep 4, 2016

Choose a reason for hiding this comment

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

Both functions are needed to so that the theme supports custom-menu. The check would be false !== strpos( $file_content, 'register_nav_menu' ) && false !== strpos( $file_content, 'wp_nav_menu' )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I will fix this in the next update.

- modified __construct() to use either the themecheck plugin or standalone cli
- modified the Class to appropriately handle child themes.
- themecheck plugin can enable combining parent theme with child theme but the standalone cli version will only sniff the child theme files.
- switched the $sniff_helper from a global variable to direct access.
- modified get_theme_supports() so that both wp_nav_menu() and register_nav_menu() must be used for custom menus to be supported.
- modified get theme supports to include register_nav_menus.
  - removed get_custom_menu_checks()
  - this will be handled in get_theme_supports()

- modified get_title_tag_check() to look for 'wp_title(' instead of 'wp_title'
 so that use of 'wp_title' in add_filter() is not affected
- added no title-tag support error
- added no wp_title() allowed error
}
}
if ( preg_match_all( '#\$(\S)*\s*=\s*get_post_meta#' , $file_content, $matches3, PREG_SET_ORDER ) ) {
echo 'wtf';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OOPS :)

@grappler grappler force-pushed the feature/theme-review-sniffs branch 2 times, most recently from 563cd1d to 6671823 Compare September 27, 2016 12:28
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.

5 participants