-
Notifications
You must be signed in to change notification settings - Fork 37
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
New sniff: Verify that an up-to-date version of TGMPA is being used. #132
Conversation
25afb76
to
fa73a4c
Compare
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. |
b98ca3f
to
d96b8c1
Compare
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. |
Can you please post result in gist? We can inform theme author to update. |
@jrfnl is it possible to get full path in the report. In some messages, file name is truncated and there is no theme name. |
@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. |
@ernilambar The gist has been updated. |
@jrfnl Thanks. Before commenting in the trac tickets, I need to fix a theme of my own. 😄 |
Note: themes are (currently) not required to use a single pefix. |
@ernilambar Thanks for starting the update drive! 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 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. |
@ernilambar Ok, as I knew it would, it took a while, but all done again. 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 summaryResults based on running the sniff over the 4601 currently published themes on wordpress.org (126.093 files analysed) per April 11 2017
|
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 |
@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 ? |
Ah, ok. I removed it because I dont like it. :D I like version in this format |
@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 ;-) |
bbdb676
to
92684e0
Compare
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 summaryResults based on running the sniff over the 4900 currently published themes on wordpress.org (139.751 files analysed) per August 4 2017.
I've also added metrics to the sniff now to get some nice statistics:
|
0c069e7
to
171801b
Compare
Rebased & added new commit with the relevant updates for PHPCS 3.x and consistency with WPCS. |
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.
171801b
to
4e85931
Compare
…he TGMPA test files
There was a problem hiding this 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.
This sniff tries to find the TGM Plugin Activation library and if this is found, will check that:
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.