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

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

Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Apr 8, 2017

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

  • the included class is up-to-date
  • isn't an unstable (alpha/beta) pre-release
  • if the required_flavour property is set, that the library was downloaded using the Custom TGMPA generator with the correct settings (warning)

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 WordPress/WordPress-Coding-Standards#912. A backmerge would solve it for this fork too.

@jrfnl jrfnl requested a review from grappler April 8, 2017 21:21
@jrfnl jrfnl force-pushed the feature/check-correct-tgmpa-version branch 2 times, most recently from 25afb76 to fa73a4c Compare April 8, 2017 21:51
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 8, 2017

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:

  • 340 themes use an out-of-date version of TGMPA (out of ~550 themes total using TGMPA)
  • 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.
  • 57 themes contain the manual search & replace error

@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.

@jrfnl jrfnl force-pushed the feature/check-correct-tgmpa-version branch 2 times, most recently from b98ca3f to d96b8c1 Compare April 9, 2017 00:40
@joyously
Copy link

joyously commented Apr 9, 2017

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
Copy link
Contributor Author

jrfnl commented Apr 9, 2017

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
Copy link
Member

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

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 9, 2017

@ernilambar
Copy link
Member

@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
Copy link
Contributor Author

jrfnl commented Apr 9, 2017

@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
Copy link
Contributor Author

jrfnl commented Apr 9, 2017

@ernilambar The gist has been updated.

@ernilambar
Copy link
Member

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

@carolinan
Copy link

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

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 11, 2017

@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
Copy link
Contributor Author

jrfnl commented Apr 12, 2017

@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
Copy link
Member

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
Copy link
Contributor Author

jrfnl commented Apr 13, 2017

@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
Copy link
Member

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

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 13, 2017

@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
Copy link
Contributor Author

jrfnl commented Aug 5, 2017

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 jrfnl force-pushed the feature/check-correct-tgmpa-version branch 2 times, most recently from 0c069e7 to 171801b Compare May 27, 2018 12:29
@jrfnl
Copy link
Contributor Author

jrfnl commented May 27, 2018

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

jrfnl and others added 3 commits May 31, 2018 13:40
This sniff tries to find the TGM Plugin Activation library and if this is found, will check that:
* the included class is up-to-date
* isn't an unstable (alpha/beta) pre-release
* if the `required_flavour` property is set, that the library was downloaded using the Custom TGMPA generator with the correct settings (warning)

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.
@jrfnl jrfnl force-pushed the feature/check-correct-tgmpa-version branch from 171801b to 4e85931 Compare May 31, 2018 11:40
Copy link
Member

@grappler grappler left a comment

Choose a reason for hiding this comment

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

Looks good. Can't think of any issues.

@jrfnl jrfnl merged commit 564f6d5 into feature/theme-review-sniffs May 31, 2018
@jrfnl jrfnl deleted the feature/check-correct-tgmpa-version branch May 31, 2018 17:43
@jrfnl jrfnl added this to the 0.1.0 milestone Aug 20, 2018
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