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 sniffs for the theme checks #578

Closed
grappler opened this Issue Jul 1, 2016 · 16 comments

Comments

Projects
None yet
8 participants
@grappler
Contributor

grappler commented Jul 1, 2016

I am opening this issue after discussing this with @GaryJones, @westonruter, @fklein-lu, @jrf & @Rarst at the WCEU contributors day and showing this to the theme review admins and @samuelsidler before publishing. This is one part of the project to improve the theme review process.

Action plan:

  • Add the open issues from the Theme Check repo for rules which haven't got a check to this list.
  • [Extra] Add a check for up-to-date version of TGMPA if TGMPA is detected.
  • Get the theme review board to look over the list and where necessary correct it.
  • Move ownership of the GH Theme Check plugin repo to the WordPress organisation on GH.
  • Get people on the theme review team to create sniff unit tests for every single item on the list.
  • For each item for which there are unit tests, someone could start creating the actual sniff.
  • For any finished sniff, the 'old' check can be removed from the TC plugin.
  • Once the first batch of sniffs are created, - in a develop branch, set up TC to use PHPCS and WPCS for at least part of the checks and start working on a deploy script which pulls in the dependencies via composer.

We'd need two new rulesets:

  • Parent-theme
  • Child-theme, were child-theme would basically be a subset of parent-theme with only the 'negative' (you are not allowed to...) rules.

A number of these sniffs are not - like a typical sniff - valid for each file, but need to sniff if something is sone at least once in any of the files. So we might need to add a wrapper for those particular sniff in the Theme Check bootstrap to run those against each file and only report if a positive/negative results was returned for all.

Rules which can probably be turned into a sniff:

  • ERROR | Check that capabilities are used not roles. Functions to check: get_role(), current_user_can(), current_user_can_for_blog(), user_can(), add_..._page(). WPTRT/WPThemeReview#27 / PR: WPTRT/WPThemeReview#36
  • ERROR | Check that only add_theme_page() is used. Other add_..._page() functions not allowed. WPTRT/WPThemeReview#11 / PR: WPTRT/WPThemeReview#13
  • ERROR | 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. WPTRT/WPThemeReview#1
    • WARNING | Code similar to remove_action( 'wp_head', '_admin_bar_bump_cb' );
    • WARNING | CSS check for manipulating the styling of the admin bar with for instance display: none CSS identifiers: #wpadmin and .show-admin-bar
      These last two are suggested as warning as they will be less precise and need manual verification.
  • ERROR | Forbidden functions which include: eval(), ini_set, popen(), proc_open, exec(), shell_exec(), system(), passthru(), base64_decode(), base64_encode, uudecode(), str_rot13(). List need proper review when creating the sniff (also look at the original regexes in the Theme Check plugin) WPTRT/WPThemeReview#9 / PR: WPTRT/WPThemeReview#10
    This can probably be an extended class of one of the related checks like WordPress.PHP.DiscouragedFunctions or Generic.PHP.ForbiddenFunctions.
  • ERROR | Javascript - check for similar functions like the ones listed above. WPTRT/WPThemeReview#92
  • ERROR | Advertising/tracking codes in both PHP as well as JS and CSS and any other potential files which contain this type of code. Things like cx=[0-9]{21}:[a-z0-9]{10} or pub-[0-9]{16} WPTRT/WPThemeReview#125
  • WARNING | Using a CDN is discouraged. All JS and CSS should be bundled. For a list of typical source strings to look for, see Theme-Check plugin - /checks/cdn.php WPTRT/WPThemeReview#20 / PR: WPTRT/WPThemeReview#64
  • ERROR | Check for usage of deprecated WP constants. WPTRT/WPThemeReview#23 / PR: WPTRT/WPThemeReview#30
    For details, see https://github.com/Otto42/theme-check/pull/162/files
    We could probably extend an existing sniff of similar ilk for this.
  • ERROR | Ensure that all calls the ->add_setting() for the Customizer include a sanitize_callback or a sanitize_js_callback parameter and that it's non-empty.
    An exception will need to be made for the (two) calls found in Kirki_Settings as they do correctly comply, but are wrappers for the real call. WPTRT/WPThemeReview#68 / PR: WPTRT/WPThemeReview#76
  • ERROR | Check for usage of deprecated WP functions (required).
    For details, see Theme-Check plugin - /checks/deprecated.php #576 / PR: #633
    We could probably extend an existing sniff of similar ilk for this.
  • WARNING | Check for usage of deprecated WP functions (recommended - only for recent changes, i.e. deprecation within the last three WP versions). #576 / PR: #633
    How many versions back will need to be checked with the theme review board and might actually need to be challenged as the current 2/3 versions is based on when WP was only updated approx once a year from what I gather.
    For details, see Theme-Check plugin - /checks/dep_recommended.php
    We could probably extend an existing sniff of similar ilk for this.
  • WARNING | Verify that wp_deregister_script() isn't called. WPTRT/WPThemeReview#21 / PR: WPTRT/WPThemeReview#26
    This is basically meant to only check that core scripts aren't being deregistered, however maintaining a list of core scripts for that purpose would be a maintenance nightmare, so returning a warning when any such call is encountered is the current solution.
  • ERROR | Verify that the theme is not auto-generated, see Theme-Check plugin - /checks/generated.php for sample phrases to search for.
    Discussion needed in the Theme review board on whether this lists needs to be expanded with other generators and if so, which. WPTRT/WPThemeReview#63 / PR: WPTRT/WPThemeReview#65
  • WARNING | Verify that no favicon / Apple icon / Windows tile / Android whatever they call it is being added from the theme. The current check is in Theme-Check plugin - /checks/favicon.php, but could definitely use some fine-tuning and improvement. WPTRT/WPThemeReview#70 / PR: WPTRT/WPThemeReview#78
  • ERROR | Verify that the i18n functions are used properly => use existing sniff from WPCS. WPTRT/WPThemeReview#31 / PR: WPTRT/WPThemeReview#32
  • WARNING | Check for the use of <iframes (often used for malicious code). WPTRT/WPThemeReview#22 / PR: WPTRT/WPThemeReview#109
  • WARNING (manual check required) | Check if a theme uses include(_once) or require(_once) (where they should use get_template_part()). WPTRT/WPThemeReview#66 / PR: WPTRT/WPThemeReview#67
    Current implementation excluded the functions.php file from this check. We may want to continue doing so.
  • ERROR | Verify that line endings are LF only. Using Generic.Files.LineEndings should be sufficient for this. WPTRT/WPThemeReview#3 / PR: WPTRT/WPThemeReview#7 WPTRT/WPThemeReview#90
  • WARNING | Verify that no hard-coded URLs are found. Manual verification necessary.
    An exception is made for the Author URI and (Theme) URI as set in the style.css header as well as links to wordpress.org. WPTRT/WPThemeReview#14 / PR: WPTRT/WPThemeReview#28
    Links in the text of PHP/JS comments should be excluded from this check.
  • WARNING | Similar to deprecated/forbidden functions check, verify that file system calls use the WP_Filesystem method and not PHP native functions. WPTRT/WPThemeReview#71
    For a list of functions to trigger on, see Theme-Check plugin - /checks/malware.php
  • ERROR | Check for usage of deprecated WP functions and provide alternative based on the parameter passed. WPTRT/WPThemeReview#72 / PR: WPTRT/WPThemeReview#80
    For details, see Theme-Check plugin - /checks/more_deprecated.php
    We could probably extend an existing sniff of similar ilk for this or even combine this with the other deprecated checks.
  • ERROR | In the (register|wp)_nav_menu() function, the theme should only use a variable for the menu name. No hard-coded menu names allowed. WPTRT/WPThemeReview#93
  • WARNING | Check for non-printable characters [\x00-\x08\x0B-\x0C\x0E-\x1F\x80-\xFF]. Quite possibly there will be an existing sniff somewhere we could re-use for this. WPTRT/WPThemeReview#94
  • ERROR | No short open tags allowed. = the existing Generic.PHP.DisallowShortOpenTag sniff. WPTRT/WPThemeReview#2 / PR: WPTRT/WPThemeReview#6
  • ERROR | No alternative opening tags allowed. WPTRT/WPThemeReview#15 / PR: WPTRT/WPThemeReview#16
    Check if there is an existing sniff for the alternative opening tags, if not, I'd suggest opening a PR upstream at PHPCS to cover this in a separate sniff in Generic or add to extend the above mentioned sniff to cover this.
  • ERROR | No short open tags allowed for XML.
    Extend the existing Generic.PHP.DisallowShortOpenTag sniff to an XML specific one. WPTRT/WPThemeReview#95
  • ERROR | The following three functions are not allowed (plugin territory): register_post_type(), register_taxonomy(), add_shortcode(). WPTRT/WPThemeReview#73 / PR: WPTRT/WPThemeReview#81
    Review this list with the Theme Review board as there might be some more functions which could be added.
    The sniff could probably just extend the Forbidden Functions sniff - though it should be kept as a separate sniff for clarity.
  • ERROR | check that no include calls to searchform.php are found, if they are, recommend using get_search_form() instead. WPTRT/WPThemeReview#74 / PR: WPTRT/WPThemeReview#82
  • ERROR | Check specifically against style.css whether the required headers are found. See Theme-Check plugin - /checks/style_needed.php for the list.
  • WARNING | Check specifically against style.css whether the two recommended headers are found. See Theme-Check plugin - /checks/style_suggested.php for the list.
  • ERROR | Check that any Tags in the style.css file header comply with the current guidelines (allowed tags, deprecated tags (=> WARNING) etc). See Theme-Check plugin - /checks/style_tags.php for the list.
  • WARNING | Check if the accessibility tag is used in the style.css file header tag list and add a warning that an accessibility review is needed. WPTRT/WPThemeReview#24
  • ERROR | Check for incorrect usage of certain parameters in WP functions (hard-coded) and provide valid alternative based on the parameter passed. WPTRT/WPThemeReview#96
    For details, see Theme-Check plugin - /checks/time_date.php
  • ERROR | Check that <title.. isn't used except for in inline SVG code. WPTRT/WPThemeReview#25 / PR: WPTRT/WPThemeReview#41
  • ERROR | Check that there are no calls to wp_title(). WPTRT/WPThemeReview#97
  • ERROR | Check in style.css for the Author URI and Theme URI and verify that these are not the same.
  • ERROR | Verify - in style.css that the Theme URI does not point to wordpress.org (with predefined list of themes which are exempt and live under the wordpressdotorg user or have a check based on Author name - see the current check in checks/uri.php).
  • ERROR | Verify a number of typical php snippets which are known as malware indicators, see Theme-Check plugin - /checks/worms.php WPTRT/WPThemeReview#98

Rules which can probably be turned into a sniff but would need to be run against every file before a positive/negative result can be determined:

  • ERROR | Verify that an add_theme_support() call is made for any feature the theme has been tagged with from the following list: custom-background, custom-header, custom-menu, featured-images/post-thumbnails, post-formats, custom-logo
  • ERROR | Check that the comment reply script is being enqueued (comments should always be supported by themes).
  • ERROR | Check that the comment_reply string or rather any HTML identifiers needed for the JS script to work are present (need more info) (comments should always be supported by themes, enqueuing the script alone is not enough)
  • ERROR | Check that comment pagination is supported. At least one of the following functions would need to be found in at least one of the template files, fail if none are found at all. paginate_comments_links(), the_comments_navigation(), the_comments_pagination(), next_comments_link() or previous_comments_link()
  • Similar to the above a check is needed that the 'threaded-comments' theme tag is only used if the theme actually supports threaded comments.
  • ERROR | Check that - normally in functions.php, but could be in another file - the global variable $content_width is set, so either in the global namespace using $content_width or within a function using global $content_width; $content_width =... or $GLOBALS['content_width'].
    Note: currently the Theme Check plugin also checks for filters on embed_defaults and content_width and passes if those are found. Those checks are outdated and should not be ported.
  • ERROR | Verify that an add_editor_style() call is made if the theme has been tagged with editor-style.
  • ERROR | Verify that get_avatar() or wp_list_comments() is used at least once.
  • WARNING | Verify that there are max one link each to the author's website and one link to wordpress.org in front-end visitor facing template, e.g. footer.php or similar.
  • WARNING | Verify that (register|wp)_nav_menu() is used at least once.
    This should become an error if the theme is tagged with custom-menu.
  • ERROR | Verify that (get_post_format()|has_format() or CSS rules covering .format are found, at least once if the theme has a add_theme_support( 'post-format' ) call.
    This should become an error if the theme is tagged with post-formats.
  • ERROR | Check that post pagination is supported. At least one of the following functions would need to be found in at least one of the template files, fail if none are found at all. posts_nav_link(), paginate_links(), the_posts_navigation(), the_posts_pagination(), next_posts_link() or previous_posts_link()
  • ERROR | Verify that the_post_thumbnail()is found at least once if the theme has a add_theme_support( 'post-thumbnails' ) call.
    This should become an error if the theme is tagged with featured-image.
  • ERROR | Check if a number of specific CSS identifiers have been given styles in any of the CSS files. See Theme-Check plugin - /checks/style_needed.php for the list.
  • ERROR | Check that post tags are supported in the theme. At least one of the following functions would need to be found in at least one of the template files, fail if none are found at all. the_tags(), get_the_tag_list(), get_the_term_list()
  • WARNING | Check on the number of unique text domains encountered. If one => ok, if two => warning, if three => error.
  • ERROR | Check that add_theme_support( 'title-tag' ) is used in at least one file.
  • WARNING | Check if at least one call to register_sidebar() or dynamic_sidebar() is made.
  • ERROR | If a call to register_sidebar() is found, make sure there is at least one call to dynamic_sidebar() as well and visa versa.
  • ERROR | Check that the register_sidebar() function is called with an add_action( 'widget_init', ... ) call.
  • ERROR | Check for a number of function calls which each theme has to contain. See Theme-Check plugin - /checks/basic.php for the list.
  • ERROR | Check that the theme contains a DOCTYPE headers somewhere.

Rules which would need another solution (like in the bootstrap file which would run PHPCS from the Theme-check plugin within an install):

  • WARNING | Have a default (always on) INFO item which will warn people not to use their own functions for features which should be supported through add_theme_support().
  • ERROR | Check for extraneous directories, such as .git, .svn, .hg, .bzr. These should not exist in the repo.
    Exact list of exclusions should be re-determined in conjunction with the theme review board. This should include both system as well as development related directories.
  • ERROR | Check for extraneous files, like thumbs.db, .DS_Store, travis config, PHPCS config .zip files etc. For a list, see Theme-Check plugin - /checks/filenames.php. These should not exist in the repo. Zips are often plugin files and are not allowed to be shipped with a theme either.
    Exact list of exclusions should be re-determined in conjunction with the theme review board. Again, this should include both system as well as development related files.
  • ERROR | Check that at the very least the following two files exist: index.php and style.css.
  • WARNING | Check that a readme.txt file exists.
  • WARNING | Verify that the text-domain used is the same as the theme slug. This can't use the custom setting in the phpcs.xml file as that file will not be allowed to be included in themes (to prevent people excluding rules).
    The text-domain should also be in the style.css file header and again be consistent.
  • ERROR | Check that at least one screenshot is found.
  • ERROR | Check that the screenshot is either a jpg or png.
  • ERROR | Check that the screenshot is smaller than 1200x900, has a 4:3 size ratio.
  • WARNING | Recommend a screenshot size of 1200 x 900 if the screenshot is smaller.
@justintadlock

This comment has been minimized.

justintadlock commented Jul 5, 2016

ERROR | Verify that the_post_thumbnail() is found at least once if the theme has a add_theme_support( 'post-thumbnails' ) call. This should become an error if the theme is tagged with featured-image.

We'd also want to check against either of these two functions:

  • get_the_post_thumbnail()
  • get_post_thumbnail_id()
@jrfnl

This comment has been minimized.

Member

jrfnl commented Jul 11, 2016

FYI:

  • Initial development of these sniffs will take place in https://github.com/WPTRT/WordPress-Coding-Standards/tree/feature/theme-review-sniffs
  • Issues will be opened in the WPTRT fork for each item to keep track of them all and measure progress.
  • If you want to contribute to this effort, for now, please send in pull requests against the above mentioned branch and fork.
  • Once an initial set of sniffs has been created, the feature branch will be pulled against WPCS for review and merge.
@manishsaini1126

This comment has been minimized.

manishsaini1126 commented May 14, 2017

I SOLVE YOUR PROBLEM I HAVE DONE SAME WORK IN PAST

@Rarst

This comment has been minimized.

Contributor

Rarst commented May 17, 2017

@manishsaini1126 that reads a bit spammy, could you please clarify if you genuinely want to get involved with this. :)

@manishsaini1126

This comment has been minimized.

manishsaini1126 commented May 17, 2017

can you please briefly describe your problem

@Rarst

This comment has been minimized.

Contributor

Rarst commented May 17, 2017

The topic is described in great detail in the opening post.

@jrfnl

This comment has been minimized.

Member

jrfnl commented May 25, 2018

Hi all,

I've had a discussion with @dingo-d today about the future of this ticket and the WPTRT-CS repo.

Progress has been very slow over the past year (and a half), but should speed up significantly over the next few weeks as @dingo-d intends to have a working prototype of the new ThemeCheck plugin which would use the WPTRT-CS ruleset ready for WCEU.

Looking back over how development has flowed so far, generally speaking, any sniffs which were created for WPTRT-CS which would benefit the wider WP community - not just the TRT - have been pulled here in WPCS and included in WPTRT-CS via the ruleset.
Only the few sniffs which are really theme specific have been pulled to the WPTRT-CS fork, these currently are:

  • FileInclude - discourage use of include(_once) and require(_once) as in themes, more often than not get_template_part() should be used instead.
  • NoAddAdminPages - forbid adding admin pages anywhere but in the Theme submenu.
  • NoAutogenerate - forbids auto generated themes.
  • NoFavicon - checks for hardcoded favicons instead of using the WP core implementation.
  • NoTitleTag - forbid the use of the <title> tag as add_theme_support( 'title-tag' ) should be used instead.
  • PluginTerritory - forbid the use of functions which add functionality and should be in plugins instead, think add_shortcode() and register_post_type().

Some more will be added in the near future, but they will be along the same lines.

All these sniffs - with NoFavicon possibly being the only exception - are very theme specific and would not be applicable to Core, VIP or plugin development.

So....

The original idea was that the WPTRT-CS would be a fork in the development phase, but would merge back into WPCS once the majority of development was done. This is how things are still currently set up.

But merging WPTRT-CS back into WPCS would introduce these theme specific sniffs into the WordPress ruleset which for most users of WPCS will be undesirable.

As the new theme sniffer would use Composer anyway, the WPTRT-CS repo could be transformed to be a new standard in its own right (like the VIP repo), which would pull PHPCS and WPCS in as dependencies and can use the WPCS (and PHPCS) sniffs by referencing them in the ruleset.

This will also create more clarity for maintenance of the WPTRT-CS repo as it can be unclear now for contributors when something needs to be pulled here in WPCS and when something needs to be pulled to WPTRT-CS.
It would also allow the WPTRT-CS more freedom in creating their own categories for sniffs and dropping PHPCS 2.x support already in favour of PHPCS 3.x.

So, @dingo-d and me would like to hear some opinions, please vote for either of these options:

  1. Stay the original course - WPTRT-CS should at some point be merged into WPCS.
  2. Keep WPTRT-CS separate but as a fork of WPCS.
  3. Split off WPTRT-CS from WPCS to become an external PHPCS standard in its own right with WPCS as a dependency.

Your input is appreciated!

/cc @grappler @WordPress-Coding-Standards/wpcs-admins

@manishsaini1126

This comment has been minimized.

manishsaini1126 commented May 25, 2018

@Rarst can we discs now and sorry me not reply your msg

@jrfnl

This comment has been minimized.

Member

jrfnl commented May 25, 2018

FYI: I've re-ordered the list of sniffable rules into "Done" and "To do" to get a better overview of where this project is at.

@JDGrimes

This comment has been minimized.

Contributor

JDGrimes commented May 25, 2018

I would be OK with option 1, but number 3 may make the most sense at this point.

@GaryJones

This comment has been minimized.

Member

GaryJones commented May 26, 2018

I'm 100% behind number 3.

Just as WPCS builds on top of (has a dependency on) PHPCS by using some sniffs from it and including our own, for the benefit of the WP subsection of the PHP community, so should WPTRT-CS build on top of (have a dependency on) WPCS by using some of our sniffs and including their own, for the benefit of the TRT (and theme builders) subsection of the WP community.

@acosmin

This comment has been minimized.

acosmin commented May 26, 2018

Number 3 :)

Also, I would like to see some sniffs for:

  • wp_remote_*() functions. I've seen them used to get (sometimes post) lists of themes, changelogs and other stuff.
  • registering/displaying WordPress dashboard widgets.
@jrfnl

This comment has been minimized.

Member

jrfnl commented May 26, 2018

@acosmin Would you mind checking if there are issues open for the two sniffs you mention in the TRT fork repo ? If not, could you open issues for this there ?

@justintadlock

This comment has been minimized.

justintadlock commented May 26, 2018

wp_remote_*() functions. I've seen them used to get (sometimes post) lists of themes, changelogs and other stuff.

What sort of sniffs do we need for wp_remote_*()? We've traditionally allowed wp_remote_get() in themes for displaying on an admin page. A good example is having a "child themes" admin page for displaying all of the current theme's children. Even I use it in at least one theme.

@jrfnl

This comment has been minimized.

Member

jrfnl commented May 26, 2018

@justintadlock @acosmin Please have the discussion about wp_remote_get() in the TRT repo.

@jrfnl

This comment has been minimized.

Member

jrfnl commented Sep 23, 2018

FYI: the split off of the Theme Review repo from WPCS has been finalized.

The Theme Review CS repo can now be found here: https://github.com/WPTRT/WPThemeReview

Any work which was originally contained in WPCS and remains in TRTCS (most notably three WPCS deprecated sniffs, but also some dev files) has been cherrypicked to the new WPThemeReview develop to maintain credits and copyright.

A number of the open items mentioned in the original issue + in the comments have not (yet) been turned into issues in the TRTCS repo. If anyone from the TRT fancies doing so, it would be very helpful and very welcome indeed!

@jrfnl jrfnl closed this Sep 23, 2018

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