WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
209 stars 37 forks source link

New sniff: Verify that an up-to-date version of TGMPA is being used. #132

Closed jrfnl closed 6 years ago

jrfnl commented 7 years ago

This sniff tries to find the TGM Plugin Activation library and if this is found, will check that:

It also checks for a persistent manual search & replace error I have seen way too often which causes white screens of death (fatal error) for end-users.

Includes extensive unit tests.

/cc @GaryJones


N.B.: the build failure is unrelated to this PR and has already been solved upstream via https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/912. A backmerge would solve it for this fork too.

jrfnl commented 7 years ago

Additional testing done:

I've run this sniff over a complete mirror of the theme directory ( ~4500 themes with 125.000+ files) and the detection seems to be quite correct. I'm still going through the results, but it's looking good so far.

The results themselves however do not look good:

@grappler Is there any way we can force the authors of the 57 themes which cause fatal errors to upgrade TGMPA ?

If you like, I can post a copy of the results of the run in a gist.

joyously commented 7 years ago

43 out of these, use a version stripped of credits and with their own class/function prefixes which should not be used no matter what.

Could this be partly the TRT's fault, since "themes should use only one unique prefix"?

jrfnl commented 7 years ago

43 out of these, use a version stripped of credits and with their own class/function prefixes which should not be used no matter what.

Could this be partly the TRT's fault, since "themes should use only one unique prefix"?

Well, I've not come across that message very often in reviews.

The manual search & replace error however, is most definitely "taught" behaviour caused by the "you should only use one text-domain" messages. Still, anyone with even the most meager knowledge of PHP would know not to do a blind search & replace, but that clearly hasn't stopped some. I see about 3 forum posts a week about fatal errors caused by this....

ernilambar commented 7 years ago

Can you please post result in gist? We can inform theme author to update.

jrfnl commented 7 years ago

@ernilambar Here you go: https://gist.github.com/jrfnl/2c23df735bd797d943ec08459ce5a76a

ernilambar commented 7 years ago

@jrfnl is it possible to get full path in the report. In some messages, file name is truncated and there is no theme name.

jrfnl commented 7 years ago

@ernilambar I'll rerun the report with a ridiculous report-width setting and see if that helps ;-)

In the mean time, I've added an error code summary to the report.

jrfnl commented 7 years ago

@ernilambar The gist has been updated.

ernilambar commented 7 years ago

@jrfnl Thanks. Before commenting in the trac tickets, I need to fix a theme of my own. :smile:

carolinan commented 7 years ago

Note: themes are (currently) not required to use a single pefix.

jrfnl commented 7 years ago

@ernilambar Thanks for starting the update drive! I just saw the response in https://themes.trac.wordpress.org/ticket/39662#comment:4 which got me puzzled.

The mirror I ran the sniff on is a local copy of the Theme repository created with: https://github.com/jrfnl/WordPress-Theme-Directory-Slurper (windows compatible version based on https://github.com/aaronjorbin/WordPress-Theme-Directory-Slurper/)

I updated the mirror just before running the sniff, but the local copy of the aReview theme is v 1.0.1 (last updated on the local mirror in January 2017), while according to the repository, the latest version is v 1.1.0 updated Feb 2017.

So I suspect some hickups in the API now and again. Possibly to do with themes which have been updated, but the theme not set live yet/approved. I suspect that in those cases the API does indicated that the theme has been updated, but that the download would be refused and the actual update failed.

All I can think of, is to erase my local mirror completely and start a complete new download of the theme repo (which will take a while) and run the sniff over the new copy in a few days once the download is finished.

jrfnl commented 7 years ago

@ernilambar Ok, as I knew it would, it took a while, but all done again. I've created a completely new fresh mirror of the Theme repository and ran the sniff over it.

The results are only slightly better, but not much. I've updated the gist with the new results: https://gist.github.com/jrfnl/2c23df735bd797d943ec08459ce5a76a

Result summary

Results based on running the sniff over the 4601 currently published themes on wordpress.org (126.093 files analysed) per April 11 2017

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE COUNT
----------------------------------------------------------------------
WordPress.Theme.CorrectTGMPAVersion.wrongVersion 311
WordPress.Theme.CorrectTGMPAVersion.upgradeRequired 285
WordPress.Theme.CorrectTGMPAVersion.ManualEditDetected 56
WordPress.Theme.CorrectTGMPAVersion.versionUndetermined 43
----------------------------------------------------------------------
A TOTAL OF 695 SNIFF VIOLATIONS WERE FOUND IN 373 FILES
----------------------------------------------------------------------
ernilambar commented 7 years ago

WARNING | You are required to use a version of the TGM Plugin Activation library downloaded through the Custom TGMPA Generator. Download a fresh copy and make sure you select "WordPress.org" as your publication channel to get the correct version. http://tgmpluginactivation.com/download/ (WordPress.Theme.CorrectTGMPAVersion.wrongVersion)

@jrfnl In my theme I got this warning but TGM was good and generated for WordPress.org. Only difference was @version 2.6.1 for parent theme Mediclean for publication on WordPress.org. In my theme I had kept @version 2.6.1.

jrfnl commented 7 years ago

@ernilambar The only way to tell whether TGMPA was downloaded via the custom generator with the correct settings - i.e. "for publication on wordpress.org" - is by checking the version tag.

Just out of curiosity: why did you remove the added info from the version tag ?

ernilambar commented 7 years ago

Ah, ok. I removed it because I dont like it. :D I like version in this format x.x.x .

jrfnl commented 7 years ago

@ernilambar I originally put it in to make it easier for Theme reviewers to see whether the correct TGMPA version was being used. If the header did not contain the "for publication on wordpress.org", they'd need to check more carefully. Automating that check basically works the same way ;-)

jrfnl commented 7 years ago

I've done a new run over the theme repository. The full results have been put in a gist again: https://gist.github.com/jrfnl/dda0cb79a9381e5b0564dd872df5bfe6

Result summary

Results based on running the sniff over the 4900 currently published themes on wordpress.org (139.751 files analysed) per August 4 2017.

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
WordPress.Theme.CorrectTGMPAVersion.wrongVersion                 276
WordPress.Theme.CorrectTGMPAVersion.upgradeRequired              249
WordPress.Theme.CorrectTGMPAVersion.ManualEditDetected           45
WordPress.Theme.CorrectTGMPAVersion.versionUndetermined          43
----------------------------------------------------------------------
A TOTAL OF 613 SNIFF VIOLATIONS WERE FOUND IN 334 FILES
----------------------------------------------------------------------

I've also added metrics to the sniff now to get some nice statistics:

PHP CODE SNIFFER INFORMATION REPORT
----------------------------------------------------------------------
Publication Channel:
    WordPress.org => [353/629, 56.12%]
    n/a           =>  272 (43.24%)
    ThemeForest   =>    4 (0.64%)

Used in:
    parent theme  => [348/629, 55.33%]
    unknown       =>  273 (43.4%)
    child theme   =>    8 (1.27%)

Version:
    2.6.1         => [380/672, 56.55%]
    2.5.2         =>  138 (20.54%)
    2.4.0         =>   55 (8.8%)
    unknown       =>   43 (6.4%]
    2.3.6         =>   23 (3.42%)
    2.5.1         =>    9 (1.34%)
    2.5.0-alpha   =>    8 (1.19%)
    2.4.1         =>    6 (0.89%)
    2.4.2         =>    4 (0.60%)
    2.6.0         =>    3 (0.45%)
    2.5.0         =>    2 (0.3%)
    2.5.2.1       =>    1 (0.15%)

Manual editing detected:
    no            => [558/603, 92.54%]
    yes           =>   45 (7.46%)
----------------------------------------------------------------------
jrfnl commented 6 years ago

Rebased & added new commit with the relevant updates for PHPCS 3.x and consistency with WPCS.