Support phpcodesniffer-composer-installer for easy integrate code style #855

alexander-akait · 2017-02-27T14:31:33Z

https://github.com/DealerDirect/phpcodesniffer-composer-installer

JDGrimes · 2017-02-27T15:50:51Z

Related: #546

\cc @schlessera

jrfnl · 2017-02-28T20:47:52Z

@Rarst Would you have an opinion on this ?

Rarst · 2017-03-06T17:23:45Z

Oops, sorry my notifier was broken for some days and I missed bunch of notifications.

I am not sure what the installer does in this case. I remember seeing one that copied stuff around and I don't like that approach. In my opinion Composer packages should never be nested and specific case of standards should be handled with configuration rather than location.

alexander-akait · 2017-03-06T17:45:20Z

@Rarst this only set installed_paths config variable right

Rarst · 2017-03-06T18:04:44Z

Ah, good to hear. :) I will try to have a proper look at it and play with later then.

alexander-akait · 2017-03-21T12:52:19Z

/cc @Rarst

Potherca · 2017-04-24T18:09:00Z

As this litterally only adds "type" : "phpcodesniffer-standard", to the composer.json, could this PR be looked at?

(There is further documentation outlining the rational behind this change if that helps...)

JDGrimes · 2017-04-24T19:05:05Z

I think this makes sense, I can see no downside. I do want to confirm that this will work properly though, since WPCS is actually a bundle of several standards (WordPress, WordPress-Core, etc.) ??

#546 could then be closed, as I'd expect that if PHPCS ever does add native support for something like this (which seems unlikely at this point), it would follow the same conventions already being employed by existing plugins.

Rarst · 2017-04-24T19:37:40Z

I am getting to looking at it properly this week, will report. :)

Rarst · 2017-04-30T17:00:05Z

Ok, finally got to it.

So given this test composer.json

{
	"require" : {
		"squizlabs/php_codesniffer" : "^2.0",
		"dealerdirect/phpcodesniffer-composer-installer" : "0.3.2",
		"wp-coding-standards/wpcs":"dev-patch-1"
	},
	"repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/evilebottnawi/WordPress-Coding-Standards"
        }
    ]
}

Running composer install --no-dev will net us following vendor/squizlabs/php-codesniffer/CodeSniffer.conf

<?php
 $phpCodeSnifferConfig = array (
  'installed_paths' => 'C:\\server\\phpcs-installer-test\\vendor/wp-coding-standards/wpcs',
)
?>

I like this approach:

just works (no scripting in composer.json needed)
doesn't move standards files around
produces reliable absolute path to standards in config

One note on the PR itself — it sets type, but doesn't require installer plugin itself. So it would need either for plugin to be otherwise required or installed globally. If we want to go with this then we should probably require plugin ourselves as well.

Notably this will not work on checkout of this repository itself (as opposed to be pulled in as dependency) since installer doesn't seem to currently handle the case (see PHPCSStandards/composer-installer#20 ) so we would have to keep scripts setup in composer.json we already have.

Rarst

Should require or suggest the installer plugin.
Should remove scripts set up if require is used.
Could update documentation if suggest is used.

Potherca · 2017-05-02T11:31:31Z

@Rarst Should we fix PHPCSStandards/composer-installer#20 before this PR (possibly including the require) gets merged?

Rarst · 2017-05-02T11:49:43Z

That would be cleaner overall, but I don't think it's critical since we currently already handle root case ourselves with composer.json script instructions https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/5dbac380f4c0b8c61640b42825cc91034ce2a77b/composer.json#L21-L22

jrfnl · 2017-05-03T14:30:34Z

Possibly superfluous, but this issue provides some more information about the cases the DealerDirect composer plugin handles: PHPCSStandards/composer-installer#24

frenck · 2017-05-11T08:26:44Z

@Rarst PHPCSStandards/composer-installer#20 has been closed and a new release is out.

The plugin is now able to handle the root case.

GaryJones

In terms of the single-line change here, I've got no objection to it being added - it's be up to the external packages to make use of it correctly. On it's own, it's just a line in composer.json that hopefully won't cause any issues to packages that read type, but don't understand phpcodesniffer-standard.

Potherca · 2017-05-11T10:44:28Z

@GaryJones As mentioned in the rational linked in my earlier comment: The type should not cause any problems as the specified behaviour for custom-installers is to ignore any type that composer (or any composer plugin) does not recognize and default to "library".

JDGrimes · 2017-05-11T13:29:22Z

So the PR should now be updated, as @Rarst requested, to:

require dealerdirect/phpcodesniffer-composer-installer.
Remove the scripts section as it is then no longer needed at all.

My only question is:

I was under the impression that there are several different installers for PHPCS standards. This one seems to be well maintained, so I'm fine with using/depending on it. But I would just like to be clear about what affects, if any, this might have on somebody who uses a different installer. I have a feeling if somebody installed WPCS with a different installer, us requiring this installer might cause issues/conflicts—am I correct?

Rarst · 2017-05-11T14:07:41Z

I have a feeling if somebody installed WPCS with a different installer, us requiring this installer might cause issues/conflicts—am I correct?

Given that there is no official way (I think) for installers to claim type for themselves, two installers trying to target same type will likely conflict to some degree.

We could not add the specific plugin to require, more compatible less convenient.

frenck · 2017-05-11T14:12:30Z

@Rarst Correct, there is no official way to claim it.

My suggestion in that case (when going for the more 'safe' route) would be:

Do not require the installer, for compatibility sake.
Add plugin package suggestion in the composer.json
Keep old script instructions (should not be a problem if the plugin is used).
Update documentation

You can still decide (over time), what to do.

JDGrimes · 2017-05-11T14:13:21Z

We could not add the specific plugin to require, more compatible less convenient.

I think being more convenient makes sense here, but I just wanted to be clear that it would potentially have compatibility issues for some people at some point. If it ever were to become a significant issue, we could revisit it.

Rarst · 2017-05-11T14:15:55Z

From quick search of GH the common practice seems to be declare phpcodesniffer-standard type, but not require specific plugin. I would guess some of those might not even intend for plugin to be used, might just be filling in the type for information.

JDGrimes · 2017-05-11T14:17:33Z

Posted my last comment at the same time as @frenck. On second thought I'd say he is probably correct, and we might want to just suggest the use of the plugin in the README for now, to ease installation.

So this PR would be merged as-is, and then the install instructions would be updated to mention this option.

Rarst · 2017-05-11T14:22:03Z

So this PR would be merged as-is

I'd like to see suggest added, since other plugin I've seen is way more messy IMO than this one. Won't hurt to include docs in PR as well.

PS oh dear, we just keep picking on this. :) if @evilebottnawi sick of it I can finish this up [next week].

Potherca · 2017-05-11T19:25:37Z

I would also suggest not adding a require just yet. The type (and possibly a suggest) should be fine.

A require can always be added later. That would also give us time to look at the "conflicting installers" scenario and ways of dealing with it in the plugin, as that is bound to show up anyway...

jrfnl

To summarize the current status:

This PR needs to be updated as per @frenck's comment:

Add suggest section to the composer.json suggesting the dealerdirect/phpcodesniffer-composer-installer plugin.
Adjust the readme install instructions for Composer to allow the user to make their own choice from the different Composer plugins available which all use this same type.

https://github.com/DealerDirect/phpcodesniffer-composer-installer

Fixes the open actions for PR 855.

jrfnl · 2017-06-30T00:30:35Z

I've rebased & updated the PR. @Rarst @GaryJones Would you mind having another look ?

Superseded by new commit

jrfnl · 2017-06-30T00:45:05Z

Closes #546

GaryJones · 2017-06-30T06:59:22Z

composer.json

@@ -13,12 +13,16 @@
 	"require"    : {
 		"squizlabs/php_codesniffer": "^2.9.0"
 	},
+	"suggest" : {
+		"dealerdirect/phpcodesniffer-composer-installer": "*"


Worth suggesting the alternative as well?

Wouldn't that create the risk of people blindly installing everything that's suggested and then having two plugins doing the same thing (and possibly conflicting with each other) ?

That was the reason for me to just suggest one anyway.

alexander-akait · 2017-06-30T09:26:43Z

Guys, sorry, i don't have time to this now, feel free to close this PR and create new if something wrong

jrfnl · 2017-06-30T10:33:22Z

@evilebottnawi No worries, we appreciate you taking the initiative for this and getting the discussion going. I've updated your PR (so you'll get whuffies for your work) and hope to get this merged soon.

GaryJones · 2017-06-30T10:40:53Z

README.md

+
+There are two actively maintained Composer plugins which can handle the registration of standards with PHP_CodeSniffer for you:
+* [composer-phpcodesniffer-standards-plugin](https://github.com/higidi/composer-phpcodesniffer-standards-plugin)
+* [phpcodesniffer-composer-installer](https://github.com/DealerDirect/phpcodesniffer-composer-installer)


Do these two both work in roughly the same way? How would someone choose between the two?

Worth WPCS picking one as the recommendation, and leaving the other as a link for an alternative, instead of presenting both equally (in this doc)?

Do these two both work in roughly the same way?

AFAIK yes, they do.

How would someone choose between the two?

Probably the same way as when choosing between two different WP plugins to use:

See if it's still actively maintained, issues are being responded to, has good reviews, how many users it has on packagist.

Install each, test it, see if you like it and it does what you want.

Worth WPCS picking one as the recommendation, and leaving the other as a link for an alternative, instead of presenting both equally (in this doc)?

We could, but I figured most Composer users would just require WPCS and see the suggest and follow that without reading the readme. I'd be happy to adjust it though.

The reason I choose the @Dealerdirect version as the recommendation is that the team of that plugin has been very active trying to help external standards get this issue sorted, created their Composer plugin in response to an issue with another external standard I'm involved with and has been very responsive to queries related to the plugin.

Potherca mentioned this pull request Apr 24, 2017

Does't work after installation by composer PHPCompatibility/PHPCompatibility#102

Closed

JDGrimes added the Type: Enhancement label Apr 24, 2017

JDGrimes added this to the 0.12.0 milestone Apr 24, 2017

Rarst suggested changes Apr 30, 2017

View reviewed changes

Rarst mentioned this pull request Apr 30, 2017

This adds support for standards installation through Composer. #546

Closed

GaryJones approved these changes May 11, 2017

View reviewed changes

jrfnl previously requested changes May 14, 2017

View reviewed changes

frenck mentioned this pull request May 31, 2017

Invalid "installed_paths" config value PHPCSStandards/composer-installer#32

Closed

jrfnl mentioned this pull request Jun 12, 2017

Release 0.12.0 ? #962

Closed

evilebottnawi and others added 2 commits June 30, 2017 00:44

Support phpcodesniffer-composer-installer for easy integrate code style

14a198a

https://github.com/DealerDirect/phpcodesniffer-composer-installer

Composer: suggest composer plugin & update README

e986171

Fixes the open actions for PR 855.

jrfnl force-pushed the patch-1 branch from 6c3520e to e986171 Compare June 30, 2017 00:29

jrfnl added the Status: Review ready label Jun 30, 2017

GaryJones reviewed Jun 30, 2017

View reviewed changes

Rarst approved these changes Jun 30, 2017

View reviewed changes

GaryJones approved these changes Jun 30, 2017

View reviewed changes

JDGrimes merged commit 3d15aa6 into WordPress:develop Jun 30, 2017

GaryJones removed the Status: Review ready label Aug 21, 2018

jrfnl mentioned this pull request Aug 21, 2018

Reorganize issue/PR labels #1454

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support phpcodesniffer-composer-installer for easy integrate code style #855

Support phpcodesniffer-composer-installer for easy integrate code style #855

alexander-akait commented Feb 27, 2017

JDGrimes commented Feb 27, 2017

jrfnl commented Feb 28, 2017

Rarst commented Mar 6, 2017

alexander-akait commented Mar 6, 2017 •

edited

Loading

Rarst commented Mar 6, 2017

alexander-akait commented Mar 21, 2017

Potherca commented Apr 24, 2017

JDGrimes commented Apr 24, 2017

Rarst commented Apr 24, 2017

Rarst commented Apr 30, 2017 •

edited

Loading

Rarst left a comment •

edited

Loading

Potherca commented May 2, 2017

Rarst commented May 2, 2017

jrfnl commented May 3, 2017

frenck commented May 11, 2017

GaryJones left a comment

Potherca commented May 11, 2017

JDGrimes commented May 11, 2017

Rarst commented May 11, 2017

frenck commented May 11, 2017 •

edited

Loading

JDGrimes commented May 11, 2017

Rarst commented May 11, 2017

JDGrimes commented May 11, 2017

Rarst commented May 11, 2017

Potherca commented May 11, 2017

jrfnl left a comment •

edited

Loading

jrfnl commented Jun 30, 2017

jrfnl commented Jun 30, 2017

GaryJones Jun 30, 2017

jrfnl Jun 30, 2017

alexander-akait commented Jun 30, 2017

jrfnl commented Jun 30, 2017

GaryJones Jun 30, 2017

jrfnl Jun 30, 2017

Support phpcodesniffer-composer-installer for easy integrate code style #855

Support phpcodesniffer-composer-installer for easy integrate code style #855

Conversation

alexander-akait commented Feb 27, 2017

JDGrimes commented Feb 27, 2017

jrfnl commented Feb 28, 2017

Rarst commented Mar 6, 2017

alexander-akait commented Mar 6, 2017 • edited Loading

Rarst commented Mar 6, 2017

alexander-akait commented Mar 21, 2017

Potherca commented Apr 24, 2017

JDGrimes commented Apr 24, 2017

Rarst commented Apr 24, 2017

Rarst commented Apr 30, 2017 • edited Loading

Rarst left a comment • edited Loading

Choose a reason for hiding this comment

Potherca commented May 2, 2017

Rarst commented May 2, 2017

jrfnl commented May 3, 2017

frenck commented May 11, 2017

GaryJones left a comment

Choose a reason for hiding this comment

Potherca commented May 11, 2017

JDGrimes commented May 11, 2017

Rarst commented May 11, 2017

frenck commented May 11, 2017 • edited Loading

JDGrimes commented May 11, 2017

Rarst commented May 11, 2017

JDGrimes commented May 11, 2017

Rarst commented May 11, 2017

Potherca commented May 11, 2017

jrfnl left a comment • edited Loading

Choose a reason for hiding this comment

jrfnl commented Jun 30, 2017

jrfnl commented Jun 30, 2017

GaryJones Jun 30, 2017

Choose a reason for hiding this comment

jrfnl Jun 30, 2017

Choose a reason for hiding this comment

alexander-akait commented Jun 30, 2017

jrfnl commented Jun 30, 2017

GaryJones Jun 30, 2017

Choose a reason for hiding this comment

jrfnl Jun 30, 2017

Choose a reason for hiding this comment

alexander-akait commented Mar 6, 2017 •

edited

Loading

Rarst commented Apr 30, 2017 •

edited

Loading

Rarst left a comment •

edited

Loading

frenck commented May 11, 2017 •

edited

Loading

jrfnl left a comment •

edited

Loading