Better cover the regex related rules. #608

jrfnl · 2016-07-16T21:13:20Z

From the handbook:

Perl compatible regular expressions (PCRE, preg_ functions) should be used in preference to their POSIX counterparts. Never use the /e switch, use preg_replace_callback instead.

It’s most convenient to use single-quoted strings for regular expressions since, contrary to double-quoted strings, they have only two metasequences: ' and .

Ref: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#regular-expressions

This can be split into three distinct rules:

Don't use POSIX functions
Don't use the /e modifier
Prefer single quoted stings.

Rule 1 was up to now not covered in core. Parts of it were covered in extra and in vip, but the implementations were inconsistent and both were incomplete.
Rule 2 was not covered at all.
Rule 3 is already covered by the single vs double quotes sniff.

This PR fixes the issues with covering rule 1 and 2 with the following two commits:

Split off POSIX Functions restrictions to its own sniff.

Removes similar checks from existing sniffs (PHP.DiscouragedFunctions and VIP.RestrictedFunctions).
Makes sure that all functions in the POSIX extension are covered by this check - they previously were not.
Add unit tests for the new sniff covering all POSIX functions.

Copy in the PregReplaceEModifierSniff from the PHPCompatibility Sniff library.

Add a new PHPCompatibility folder to the Sniff folder.
Copy in the PregReplaceEModifierSniff.
Copy in the related parent class PHPCompatibility_Sniff.
Renamed both classes to follow the PSR1 autoloading pattern.
Added a readme to the PHPCompatibility sniff folder about its usage.
Excluded the PHPCompatibility folder from the PHPCS code style checks - this way we can just drop-in replace the file (ok, except for the class renaming) when there is a new version available.

GaryJones · 2016-07-17T07:28:28Z

WordPress/Sniffs/PHP/DiscouragedFunctionsSniff.php

@@ -31,13 +31,6 @@ class WordPress_Sniffs_PHP_DiscouragedFunctionsSniff extends Generic_Sniffs_PHP_
 	 * @var array(string => string|null)
 	 */
 	public $forbiddenFunctions = array(
-		// Deprecated.


Maybe leave a comment that this is handled with the new sniff?

Leaving a comment like that in the code will become very untidy in combination with the upcoming #582 work. I'd rather suggest at some point also re-ordering the vip ruleset according to their documentation and making a note in the ruleset about rules like this which are already covered in core. Ok ?

JDGrimes · 2016-07-17T17:01:15Z

Is there no way that we can just have the PHPCompatibility sniff library as a dependency? Maybe it is best to just copy the sniff over, but I shy away from anything that might require routine maintenance. 😄

jrfnl · 2016-07-17T17:25:54Z

Is there no way that we can just have the PHPCompatibility sniff library as a dependency?

See #575 as well where I originally suggested that.

The only way I can think of would be to make it a dependency via composer and add it to the ruleset. Not sure how well that would work for the re-hashing to get PHPCS to recognize the installed standards, but I'd leave that up to @Rarst to advise on.

This is not fool-proof no matter what. Not everyone uses Composer, so this could mean people will start complaining about WPCS not working as it breaks on the missing ruleset.

Maybe it is best to just copy the sniff over, but I shy away from anything that might require routine maintenance.

As for now, for the handbook coverage, we'd only need the one sniff, that seemed like the simplest way to handle it.
As noted above, I've excluded it from the WPCS code standards check to make copy/paste maintenance as easy as possible.

If/when WPCS would want to start incorporating more sniffs from PHPCompatibility, then this should be revisited IMHO..

JDGrimes · 2016-07-17T18:19:57Z

If/when WPCS would want to start incorporating more sniffs from PHPCompatibility, then this should be revisited IMHO..

OK, sounds good to me.

jrfnl · 2016-07-17T18:56:38Z

Rebased for merge conflicts.

Rarst · 2016-07-18T07:49:18Z

I am concerned about licensing here, that library is LGPL. By copy/pasting pieces of it here (into MIT project) we are making a potential mess of that.

jrfnl · 2016-07-18T08:14:41Z

Good point. Would it help if we had formal permission of the project owner ?

Rarst · 2016-07-18T08:21:25Z

Not really, it's just a... GPL mess in such case. Using it as a library would be clear separation of stuff, which LGPL allows without issue.

jrfnl · 2016-07-25T22:26:02Z

FYI: Working on an alternative solution for the e modifier sniff.

* Removes similar checks from existing sniffs (PHP.DiscouragedFunctions and VIP.RestrictedFunctions). * Makes sure that *all* functions in the POSIX extension are covered by this check - they previously were not. * Add unit tests for the new sniff covering *all* POSIX functions.

jrfnl · 2016-07-27T12:17:22Z

What with the upcoming release and the work @grappler is doing in #633, I'm removing the commit which added the e modifier sniff from PHPCompatibility for now, so that the splitting off of the POSIX functions to their own file can be merged.

I'm working on an abstract class to detect regular expressions and concrete classes for the e modifier sniff and best practices sniffs, but that will need more time and more testing, so won't be ready for the 0.10.0 release.

JDGrimes · 2016-07-27T12:58:34Z

@jrfnl Just to confirm—this is ready for merge?

jrfnl · 2016-07-27T13:00:57Z

@JDGrimes Absolutely, it now only contains the splitting off of the POSIX check from the VIP sniff to an individual sniff and adding that rule to the core ruleset where it belongs.

The other part of the regex rules will be covered in a separate PR.

Rarst · 2016-07-27T18:14:13Z

To be clear phpcombatibility got thrown out of this, so I can stop worry about licensing stuff? ;)

jrfnl · 2016-07-27T20:29:09Z

@Rarst You can (for now) ;-)

jrfnl · 2016-08-18T09:18:04Z

All and @Rarst in particular:

I've been doing quite some work on the PHPCompatibility sniff library in the mean time and have created a number of utility functions which would also be useful for WPCS and could get rid of a sh*tload of semi-duplicate code and allow us to check certain things with higher precision.

As I created these functions - though sometimes based on existing code - and contributed them to PHPCompatibility, will it be ok license-wise if I port them over to WPCS ?

At a later time - once these functions have been used a bit more and gone through the wrangler, I'd even like to PR them to PHPCS itself.

Rarst · 2016-08-18T11:23:21Z

As I created these functions - though sometimes based on existing code - and contributed them to PHPCompatibility, will it be ok license-wise if I port them over to WPCS ?

If by "based on existing code" you mean copy/paste/modify of its code then it's a GPL derivative work and ugh ugh ugh mess for us.

I am not familiar with specific case of LGPL into GPL move to say confidently about it. As above it would be clearly fine if LGPL lib was kept as dependency, but I don't really know about mixing their code.

jrfnl · 2016-08-18T13:13:57Z

By "based on existing code", I mean inspired by. The actual code was rewritten.

All the same, I find this whole licensing thing very confusing. If I write code and choose to contribute the same code to three different projects, it now sounds like the licensing of the first project to merge the code would rule over it all. That just feels wrong and discourages from contributing at all.

I also wonder if code snippets like that can be licensed or even copyrighted at all, after all, writing it from scratch someone else will just end up with 90% the same code with maybe some minor personalizations.
Licensing IMHO is more about the library as a whole and the concept of it, rather than bits and pieces of the code itself (though I know that's not how it works, but it is how I think it should work).

Being able to re-use bits and pieces will encourage best of breed coding much more than everyone re-inventing the wheel.

On a side-note: I remember seeing somewhere a note in the WPCS code base about a piece of code which was taken from WP Core. As WP Core is GPL, according to the licensing, that code shouldn't have been allowed in in that case. Correct ?

Rarst · 2016-08-18T13:23:38Z

By "based on existing code", I mean inspired by. The actual code was rewritten.

If you wrote the code from scratch then it's fine. It's your work to be licensed as you see fit.

If I write code and choose to contribute the same code to three different projects, it now sounds like the licensing of the first project to merge the code would rule over it all.

Nope. Since it's your code you would be free to contribute it to three different project and license it differently for each one. After the code is contributed to the project it's subject to its license, but you as copyright holder of your code are free to license it differently for another project.

I also wonder if code snippets like that can be licensed or even copyrighted at all, after all, writing it from scratch someone else will just end up with 90% the same code with maybe some minor personalizations.
Licensing IMHO is more about the library as a whole and the concept of it, rather than bits and pieces of the code itself (though I know that's not how it works, but it is how I think it should work).

Yes, it's vague and often people close eyes at technicalities. My completely personal rule of a thumb — copy/paste of actual code is a line behind which it becomes a clear unarguable derivative code. Many people (WordPress, cough) would disagree though and draw a line differently.

On a side-note: I remember seeing somewhere a note in the WPCS code base about a piece of code which was taken from WP Core. As WP Core is GPL, according to the licensing, that code shouldn't have been allowed in in that case. Correct ?

Correct. If it's still the case we should get rid of it.

jrfnl · 2016-08-27T10:26:37Z

@Rarst Thanks for your detailed reply! Will keep this in mind 👍

khacoder · 2016-08-29T00:23:04Z

So we find ways to not do something rather then ways to do something?

What about dual licensing code sniffer to make it easier to expand it?

Rarst · 2016-08-29T08:17:17Z

So we find ways to not do something rather then ways to do something?

Welcome to GPL.

What about dual licensing code sniffer to make it easier to expand it?

Dual licensing makes things harder, not easier. Now you have two licenses to be compatible with at the same time.

khacoder · 2016-08-29T13:52:34Z

Actually no :) Dual licensing means either one applies, from everything that I have read.

The user would pick the one to use. In the case of GPLv2 or later versus MIT, WordPress would then pick GPLv2 or later.

Rarst · 2016-08-29T14:16:25Z

Actually no :) Dual licensing means either one applies, from everything that I have read.

Yes, user can pick which license to apply, good for them. And user can do that because project is obliged to have project's code comply with both.

Think about it. I don't like GPL? I put GPL into dual license project and say it's now MIT... Doesn't work.

khacoder · 2016-08-29T14:48:57Z

So what code in Code Sniffer is MIT specific?

Rarst · 2016-08-29T14:53:52Z

So what code in Code Sniffer is MIT specific?

All of it, as long as that's the license project declares.

I am not too keen to continue general licensing chatter in this ticket. If you have specific licensing concerns or proposals for the project please open a dedicated issue.

khacoder · 2016-08-29T15:23:17Z

Nah I won't pursue it but thanks for asking :)

jrfnl added Type: Enhancement Status: Review ready labels Jul 16, 2016

jrfnl force-pushed the WPCS/feature/cover-regex-rules branch from 3781c68 to e1ffa59 Compare July 17, 2016 00:48

jrfnl mentioned this pull request Jul 17, 2016

Ordering of the rules in the rulesets #602

Closed

jrfnl force-pushed the WPCS/feature/cover-regex-rules branch from e1ffa59 to 490b775 Compare July 17, 2016 05:00

GaryJones reviewed Jul 17, 2016
View reviewed changes

JDGrimes assigned GaryJones Jul 17, 2016

jrfnl force-pushed the WPCS/feature/cover-regex-rules branch from 490b775 to 41d4cf8 Compare July 17, 2016 18:56

jrfnl force-pushed the WPCS/feature/cover-regex-rules branch from 41d4cf8 to 3cc54fe Compare July 17, 2016 22:50

jrfnl force-pushed the WPCS/feature/cover-regex-rules branch 3 times, most recently from 094de1b to b2aad8b Compare July 19, 2016 23:09

jrfnl mentioned this pull request Jul 24, 2016

[New Sniff] Forbidden functions WPTT/WPThemeReview#9

Open

5 tasks

JDGrimes removed the Status: Review ready label Jul 25, 2016

jrfnl force-pushed the WPCS/feature/cover-regex-rules branch from b2aad8b to 7721698 Compare July 27, 2016 12:17

Remove extra tab

358bc2b

This was referenced Jul 27, 2016

Modularizing the discouraged functions #633

Merged

Question, branch to test projects against? #631

Closed

JDGrimes unassigned GaryJones Jul 27, 2016

JDGrimes added this to the 0.10.0 milestone Jul 27, 2016

JDGrimes merged commit b7ca090 into develop Jul 27, 2016

JDGrimes deleted the WPCS/feature/cover-regex-rules branch July 27, 2016 16:06

jrfnl mentioned this pull request Aug 28, 2016

Add WordPress_AbstractThemeSniff Class WPTT/WPThemeReview#53

Closed

jrfnl mentioned this pull request Jan 24, 2017

Add JS linting ? WPTT/WPThemeReview#120

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better cover the regex related rules. #608

Better cover the regex related rules. #608

jrfnl commented Jul 16, 2016

GaryJones Jul 17, 2016

jrfnl Jul 17, 2016 •

edited

Loading

JDGrimes commented Jul 17, 2016

jrfnl commented Jul 17, 2016

JDGrimes commented Jul 17, 2016

jrfnl commented Jul 17, 2016

Rarst commented Jul 18, 2016

jrfnl commented Jul 18, 2016 •

edited

Loading

Rarst commented Jul 18, 2016

jrfnl commented Jul 25, 2016

jrfnl commented Jul 27, 2016

JDGrimes commented Jul 27, 2016

jrfnl commented Jul 27, 2016

Rarst commented Jul 27, 2016

jrfnl commented Jul 27, 2016

jrfnl commented Aug 18, 2016

Rarst commented Aug 18, 2016

jrfnl commented Aug 18, 2016

Rarst commented Aug 18, 2016

jrfnl commented Aug 27, 2016

khacoder commented Aug 29, 2016

Rarst commented Aug 29, 2016

khacoder commented Aug 29, 2016

Rarst commented Aug 29, 2016

khacoder commented Aug 29, 2016

Rarst commented Aug 29, 2016 •

edited

Loading

khacoder commented Aug 29, 2016

Better cover the regex related rules. #608

Better cover the regex related rules. #608

Conversation

jrfnl commented Jul 16, 2016

GaryJones Jul 17, 2016

Choose a reason for hiding this comment

jrfnl Jul 17, 2016 • edited Loading

Choose a reason for hiding this comment

JDGrimes commented Jul 17, 2016

jrfnl commented Jul 17, 2016

JDGrimes commented Jul 17, 2016

jrfnl commented Jul 17, 2016

Rarst commented Jul 18, 2016

jrfnl commented Jul 18, 2016 • edited Loading

Rarst commented Jul 18, 2016

jrfnl commented Jul 25, 2016

jrfnl commented Jul 27, 2016

JDGrimes commented Jul 27, 2016

jrfnl commented Jul 27, 2016

Rarst commented Jul 27, 2016

jrfnl commented Jul 27, 2016

jrfnl commented Aug 18, 2016

Rarst commented Aug 18, 2016

jrfnl commented Aug 18, 2016

Rarst commented Aug 18, 2016

jrfnl commented Aug 27, 2016

khacoder commented Aug 29, 2016

Rarst commented Aug 29, 2016

khacoder commented Aug 29, 2016

Rarst commented Aug 29, 2016

khacoder commented Aug 29, 2016

Rarst commented Aug 29, 2016 • edited Loading

khacoder commented Aug 29, 2016

jrfnl Jul 17, 2016 •

edited

Loading

jrfnl commented Jul 18, 2016 •

edited

Loading

Rarst commented Aug 29, 2016 •

edited

Loading