Import several phpcs sniffs from MediaWiki #17406

anomiex · 2020-10-07T21:12:45Z

Changes proposed in this Pull Request:

MediaWiki has a large suite of custom phpcs sniffs, several of which would be useful for Jetpack.

MediaWiki.AlternativeSyntax.LeadingZeroInFloat: Prefer 0.5 over .5 for better readability.
MediaWiki.Classes.UnsortedUseStatements: Require use statements be sorted.
MediaWiki.Classes.UnusedUseStatement: Attempts to flag use statements where the used class isn’t actually used.
MediaWiki.ExtraCharacters.ParenthesesAroundKeyword: Flags things like clone( $x ), continue( 2 ), and break( 2 ), in addition to things like require_once( '...' ) that are already getting flagged by PEAR.Files.IncludingFile.BracketsNotRequired.
MediaWiki.Usage.DirUsage: Prefer __DIR__ to dirname( __FILE__ ).
MediaWiki.Usage.DoubleNotOperator: ! ! $x is "clever code", (bool) $x would be clearer.
MediaWiki.Usage.InArrayUsage: Flags constructs like in_array( $key, array_keys( $array ) ) and in_array( $key, array_flip( $array ) ) that could be done more clearly and efficiently with isset( $array[ $key ] ) or array_key_exists( $key, $array ).
MediaWiki.Usage.MagicConstantClosure: Flag use of __METHOD__ and __FUNCTION__ inside closures, as they don’t produce useful output.
MediaWiki.Usage.NestedFunctionsSniff: Defining a function (not a closure) inside a function is "clever code", if not an outright bug.
MediaWiki.Usage.PHPUnitAssertEquals: Prefer other PHPUnit assertions over assertEquals() in some cases for better type safety.
MediaWiki.Usage.PlusStringConcat: Flag attempts to concatenate strings with + rather than .. Useful for programmers coming from JS.
MediaWiki.Usage.ReferenceThis: &$this raises warnings since PHP 7.1, warn against it. Surprisingly not caught by the "PHPCompatibility" sniffs we’re importing.
MediaWiki.WhiteSpace.MultipleEmptyLines: Forbids multiple empty lines everywhere, versus Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines which only does so inside functions.
MediaWiki.WhiteSpace.SpaceAfterClosure: The WordPress sniffs seem to have good coverage of whitespace stuff, but miss that function() should be function ().

Jetpack product discussion

p9dueE-1Y9-p2

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Run composer php:lint on various files to see what it finds.

Proposed changelog entry for your changes:

Developer only, so probably not needed.

anomiex · 2020-10-07T21:17:27Z

Submitting this now as a proof of concept. If we want to go ahead with this, I'll need to poke upstream about a few things:

I had to use dev-master here since their latest release requires phpcs 3.5.5 rather than 3.5.6. We'll need them to tag a release so we can depend on a non-dev version.
Their package isn't set up for use with dealerdirect/phpcodesniffer-composer-installer, so right now the <rule ref="..." /> are using ugly paths. Hopefully they'll be amenable to a patch adding that support; I think it'd be a one-line change to add a "type" to their composer.json.

anomiex · 2020-10-08T16:12:23Z

I did some testing by running phpcbf for these rules, to see that they didn't wind up creating a flood of other rule violations or something. Then I uploaded the results at #17419 in case that would be useful to see.

kraftbj · 2020-10-15T20:42:29Z

Submitting this now as a proof of concept. If we want to go ahead with this, I'll need to poke upstream about a few things:

Any success poking upstream?

anomiex · 2020-10-16T14:21:17Z

Any success poking upstream?

I was waiting for feedback here. I'll go poke them now.

kraftbj · 2020-10-16T14:25:22Z

Sounds good; thanks!

anomiex · 2020-10-16T15:48:18Z

Their package isn't set up for use with dealerdirect/phpcodesniffer-composer-installer, so right now the <rule ref="..." /> are using ugly paths. Hopefully they'll be amenable to a patch adding that support; I think it'd be a one-line change to add a "type" to their composer.json.

Poked upstream at https://phabricator.wikimedia.org/T265735. It did turn out to be a one-line change.

kraftbj · 2020-10-19T18:28:05Z

I'd be okay merging this into the codesniffer package PR OR merging that one, then merging this immediately following, as it is now. If/when MediaWiki updates the package so we can pin a specific version, we can do that in a follow-up.

anomiex · 2020-10-19T20:37:44Z

I'd be okay merging this into the codesniffer package PR OR merging that one, then merging this immediately following

I'd prefer the latter, it seems cleaner to have the major refactor and the added functionality be separate in master so future git archaeologists can see what was what.

jetpackbot · 2020-10-19T21:23:52Z

Scheduled Jetpack release: November 10, 2020.
Scheduled code freeze: November 3, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17406

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against f2c7886

kraftbj · 2020-10-19T21:46:07Z

We'll likely need to do this https://github.com/Automattic/jetpack/blob/master/tests/setup-travis.sh#L15 for this package as well.

anomiex · 2020-10-20T14:48:34Z

We'll likely need to do this https://github.com/Automattic/jetpack/blob/master/tests/setup-travis.sh#L15 for this package as well.

Yeah, I see Travis is failing.

BTW, I wish Travis would email me when it fails on one of my PRs. It looks like it would by default, but specifying recipients unfortunately overrides that (travis-ci/travis-ci#8868). And I don't want emails about everyone's Travis failures...

jeherve · 2020-10-20T14:53:16Z

I wish Travis would email me when it fails on one of my PRs.

We've discussed this a bit over the years. I think updating the status of the PR itself (via a comment from a bot for example) would allow us to hit 3 birds with one stone:

It would send out an email to folks who rely on email for GitHub management / notifications
It would make it clear when looking at a PR that tests are failing
We could couple that to an action that would update the labels on the PR at the same time.

Edit: cross-linking to #17094, where @brbrr has already started looking into moving some of our tests to GitHub actions; doing so would allow to update the PR at the end of the action run.

anomiex · 2020-10-20T19:37:18Z

Ok, Travis is passing now (other than the flaky sync tests). I'm not entirely happy with what I had to do to make that happen, but it works.

MediaWiki has a large suite of custom phpcs sniffs, several of which would be useful for Jetpack. * MediaWiki.AlternativeSyntax.LeadingZeroInFloat: Prefer `0.5` over `.5` for better readability. * MediaWiki.Classes.UnsortedUseStatements: Require `use` statements be sorted. * MediaWiki.Classes.UnusedUseStatement: Attempts to flag `use` statements where the used class isn’t actually used. * MediaWiki.ExtraCharacters.ParenthesesAroundKeyword: Flags things like `clone( $x )`, `continue( 2 )`, and `break( 2 )`, in addition to things like `require_once( '...' )` that are already getting flagged by PEAR.Files.IncludingFile.BracketsNotRequired. * MediaWiki.Usage.DirUsage: Prefer `__DIR__` to `dirname( __FILE__ )`. * MediaWiki.Usage.DoubleNotOperator: `! ! $x` is "clever code", `(bool) $x` would be clearer. * MediaWiki.Usage.InArrayUsage: Flags constructs like `in_array( $key, array_keys( $array ) )` and `in_array( $key, array_flip( $array ) )` that could be done more clearly and efficiently with `isset( $array[ $key ] )` or `array_key_exists( $key, $array )`. * MediaWiki.Usage.MagicConstantClosure: Flag use of `__METHOD__` and `__FUNCTION__` inside closures, as they don’t produce useful output. * MediaWiki.Usage.NestedFunctionsSniff: Defining a function (not a closure) inside a function is "clever code", if not an outright bug. * MediaWiki.Usage.PHPUnitAssertEquals: Prefer other PHPUnit assertions over `assertEquals()` in some cases for better type safety. * MediaWiki.Usage.PlusStringConcat: Flag attempts to concatenate strings with `+` rather than `.`. Useful for programmers coming from JS. * MediaWiki.Usage.ReferenceThis: `&$this` raises warnings since PHP 7.1, warn against it. Surprisingly not caught by the "PHPCompatibility" sniffs we’re importing. * MediaWiki.WhiteSpace.MultipleEmptyLines: Forbids multiple empty lines everywhere, versus Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines which only does so inside functions. * MediaWiki.WhiteSpace.SpaceAfterClosure: The WordPress sniffs seem to have good coverage of whitespace stuff, but miss that `function()` should be `function ()`.

Uninstall automattic/jetpack-codesniffer for 5.6 and 7.0, since mediawiki/mediawiki-codesniffer requires 7.2+. Also, since we're uninstalling the phpcs standard, don't try to run phpcs. And we need a way to avoid having run-travis.sh run the tests for the codesniffer package when PHP is too old. Sigh.

kraftbj · 2020-10-26T13:56:50Z

@anomiex What's outstanding on this before ready for review?

anomiex · 2020-10-26T14:34:58Z

If you're still willing to merge it with the dev-master dependency, nothing. Otherwise, I should poke upstream to make a release.

edit: poked them on one of their IRC channels.

edit: They say they'll have a new version in a few minutes.

kraftbj

I'll follow up with a few PRs to implement these to prevent too much churn in a single PR.

Marking for 9.2 so we don't get caught with PHPCS updates needed during beta week. Let's merge this ASAP after code freeze.

anomiex added [Status] In Progress [Type] Janitorial [Pri] Normal labels Oct 7, 2020

anomiex requested review from kraftbj and leogermani October 7, 2020 21:12

anomiex self-assigned this Oct 7, 2020

github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Oct 7, 2020

kraftbj mentioned this pull request Oct 8, 2020

Dev: Add processor for lists on the require list for PHPCS #17408

Merged

anomiex mentioned this pull request Oct 8, 2020

Run phpbcf for added MediaWiki rules #17419

Closed

anomiex mentioned this pull request Oct 19, 2020

Add a package to hold our coding standard #17405

Merged

anomiex force-pushed the add/codesniffer-package branch from 7eec79d to cadfcaa Compare October 19, 2020 17:09

anomiex force-pushed the add/phpcs-rules-from-MediaWiki branch from 0786777 to b6d69e5 Compare October 19, 2020 17:26

kraftbj added this to the 9.1 milestone Oct 19, 2020

Base automatically changed from add/codesniffer-package to master October 19, 2020 20:39

anomiex force-pushed the add/phpcs-rules-from-MediaWiki branch from b6d69e5 to 1dc8f56 Compare October 19, 2020 21:13

anomiex force-pushed the add/phpcs-rules-from-MediaWiki branch from 494df99 to 781f334 Compare October 20, 2020 16:54

This was referenced Oct 20, 2020

PHPCS: Add use statement sorting sniff #17361

Closed

Fix use statement sorting #17362

Closed

anomiex force-pushed the add/phpcs-rules-from-MediaWiki branch 2 times, most recently from 2670c53 to 59f51a6 Compare October 20, 2020 19:06

anomiex added 2 commits October 22, 2020 11:32

anomiex force-pushed the add/phpcs-rules-from-MediaWiki branch from 59f51a6 to 8985c17 Compare October 22, 2020 15:35

jeherve added the [Package] Codesniffer label Oct 26, 2020

Merge branch 'master' into add/phpcs-rules-from-MediaWiki

0b88ad6

Use new mediawiki/mediawiki-codesniffer 32.0.0 release

f2c7886

anomiex added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Oct 26, 2020

kraftbj approved these changes Oct 26, 2020

View reviewed changes

kraftbj modified the milestones: 9.1, 9.2 Oct 26, 2020

kraftbj added [Status] Blocked / Hold and removed [Status] Needs Review This PR is ready for review. labels Oct 26, 2020

This was referenced Oct 26, 2020

Updated PHPCS: Packages and Debugger #17617

Merged

Updated PHPCS: AMP, Tests, etc #17618

Merged

Updated PHPCS: Modules, endpoints, and general files #17619

Merged

Updated PHPCS: Blocks #17620

Merged

kraftbj removed the [Status] Blocked / Hold label Oct 27, 2020

kraftbj merged commit 5f8ecf4 into master Oct 27, 2020

matticbot added the [Status] Needs Changelog label Oct 27, 2020

kraftbj deleted the add/phpcs-rules-from-MediaWiki branch October 27, 2020 20:32

jeherve removed the [Status] Needs Changelog label Oct 28, 2020

Import several phpcs sniffs from MediaWiki #17406

Import several phpcs sniffs from MediaWiki #17406

Uh oh!

Conversation

anomiex commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

anomiex commented Oct 7, 2020

Uh oh!

anomiex commented Oct 8, 2020

Uh oh!

kraftbj commented Oct 15, 2020

Uh oh!

anomiex commented Oct 16, 2020

Uh oh!

kraftbj commented Oct 16, 2020

Uh oh!

anomiex commented Oct 16, 2020

Uh oh!

kraftbj commented Oct 19, 2020

Uh oh!

anomiex commented Oct 19, 2020

Uh oh!

jetpackbot commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kraftbj commented Oct 19, 2020

Uh oh!

anomiex commented Oct 20, 2020

Uh oh!

jeherve commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anomiex commented Oct 20, 2020

Uh oh!

kraftbj commented Oct 26, 2020

Uh oh!

anomiex commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kraftbj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anomiex commented Oct 7, 2020 •

edited

Loading

jetpackbot commented Oct 19, 2020 •

edited

Loading

jeherve commented Oct 20, 2020 •

edited

Loading

anomiex commented Oct 26, 2020 •

edited

Loading

kraftbj left a comment •

edited

Loading