Add phpcs, coder 8.2.8 as --dev requirements for drupal/core

+1 for making it easier for people to install those tools!

I am in favor of this too.

I'm in favour too because it'll allow us to lock to specific versions of coder so we can manage how we get up-to-date with any changes to coder rules that we have applying to core already.

One question are dev requirements not packaged by Drupal.org for the tarball. I hope so.

This would be awesome. And yeah, dev requirements should not be packaged and 8.2.x branch doesn't have "require-dev" entry in composer.json.

It is indeed though. Phpunit is part of the official tarball.

+++ b/core/composer.json
@@ -36,10 +36,12 @@
+        "squizlabs/php_codesniffer": "~2",

This is probably should be ~2.1.

Created new issue in the infrastructure #2745355: Use "composer install --no-dev" to create tagged core packages to exclude dev requirements from packages created on Drupal.org.

+++ b/core/composer.json
@@ -36,10 +36,12 @@
+        "drupal/coder": "~8.2",

I think this needs to be fixed to a more specific version because minor releases occur and break stuff.

Let's not fix a specific version in the composer.json, that's just bad form. I guess something like ^8.2.0 or ^8.2.7 would satisfy #12 ?!

Or I guess ~8.2.0 as that will not go to 8.3, but ^8.2.0 will, IIRC. I always mix this up...

Hmm, I think ~8.2 was what @alexpott was unhappy with, AFAICT it should be ~8.2.0 or similar.

Unfortunately I think we need to rely on specific versions because we're adding new rules and tweaking existing ones all the time. If we don;t really on a specific versions then releasing a new version of coder might break HEAD (if we were running automated tests) and that makes no sense.

Atm core is compliant with 8.2.7 but it is not with 8.2.6 and if 8.2.8 was released tomorrow we would not be compliant with it.

Re @alexpott: But we have composer.lock to make sure that during automated testing the correct versions will be picked up. So when we run composer update drupal/coder and we have to fix some files, then that's arguably a violation of semver that drupal/coder is doing, but it should not in any way break our CI or packaging or anything else.

@tstoeckler that is true but we still need to 8.2.7 and up then. And when we move to 8.2.8 we should change the dependency.

OK, so ~8.2.7 then?

I only feel strongly that we don't pin the specific version in the composer.json, I don't care much about the rest.

Per @alexpott, what we are looking for is 8.2.7 <= x < 8.3 where x is the allowed version. However, ~8.2 <=> (8.2.0 <= x < 9.0.0). So we need to specify ~8.2.7 instead, as that is equal to the prior expression.

Edit: format the expression a bit for clarity

Also as an aside, but I guess the 8 as the major version of drupal/coder stems from the fact that contributed modules used to inherit the Drupal core major version on http://packagist.drupal-composer.org/, which breaks semver. Now with http://packages.drupal.org/ that's no longer the case, so maybe

drupal/coder should re-publish versions following semver properly now?

(I know that <code>drupal/coder

is not on http://packagist.drupal-composer.org/ but on https://packagist.org/ proper, but I would venture to guess that the versioning scheme is still inherited from there...)

But we can always update the version in core, if drupal/coder re-versions itself, so probably not directly related. Just wanted to note this nonetheless.

I'm trying to understand the points of @tstoeckler, @mile23 and @alexpott
Unlike external PHP packages with libraries in the case of coder we rely on the exact implementation for us being compatible with the rules.

Let's have a look at ~8.2.7. With this we would say, we are compatible with 8.2.7, 8.2.8 and above, but as #22 said, this is NOT the case. The next version would break Drupal. There is a semantic difference. We do not only care about semantic versioning, but actually about the concrete implementation in the version 8.2.7.

The version 8.2.x as mentioned by @mile23, is a bit similar to ~8.2.0, which again, would fail, when, for whatever reason, ~8.2.

The point raised by @tstoeckler in #25 would be okay in a world where we just care about working on core. When you though take into account of installing Drupal "library"
via the Drupal skeleton for example, we would want to rely on getting the exact right version, as otherwise, there would be fails. The composer.lock file would not help in that case.

OK, whatever. :-)

I still think it's completely wrong, but I don't want to hold this up any longer.

When you though take into account of installing Drupal "library"
via the Drupal skeleton for example, we would want to rely on getting the exact right version, as otherwise, there would be fails. The composer.lock file would not help in that case.

AFAICT that's 100% incorrect. If you install drupal/core via Composer e.g. via the project template and then also install drupal/code you are most likely doing that not because you at all care about what core devs are doing but because you want to run phpcs on custom modules that are in the repo. With this patch Composer will prevent you from installing e.g. version 2.7.9 of drupal/coder, because drupal/core (IMO artificially) pretends to not work with it. That is why (again, IMO) specific versions are always wrong. It seems correct, but in the end it just causes headaches for people actually using the code. And the benefit of locking the version in composer.json really is purely theoretical because we have committed the composer.lock anyway.

But again, whatever :-)

It would be awesome to have this in core, (ironically) specifically for the project template, but also for contributed modules, so let's just do this :-)

AFAICT that's 100% incorrect. If you install drupal/core via Composer e.g. via the project template and then also install drupal/code you are most likely doing that not because you at all care about what core devs are doing but because you want to run phpcs on custom modules that are in the repo.

Fair point, but dont' you want to run the exact same CS style as core does?

Well maybe I do, maybe I don't. That's the point.

There's another point that is missing. Drupal's dev dependencies are for when you develop the Drupal core project code. They are not for when you build projects. If you install them and expect them to run differently for other projects then you're doing it wrong. If a module wants its own standards - great - just create your own dependency to coder etc...

Also before this issue lands an issue we need to discuss changing how drupal.org's packager runs composer install.

Created #2756187: Don't include dev dependencies in tarball

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -136,6 +136,31 @@ public static function ensureHtaccess(Event $event) {
   /**
+   * Configures phpcs if present.
+   *
+   * @param \Composer\Script\Event $event
+   */
+  public static function configurePhpcs(Event $event) {

Meh, do we have to do this?

Can we add this to phpcs.xml.dist instead?

<config name="installed_paths" value="../../drupal/coder/coder_sniffer" />

Re #35: That will not work for different locations of vendor in drupal/drupal vs. the Composer template. (Yeah, I'm doing it wrong by using core's phpunit.xml.dist, etc., but it's still true.)

OK, but then let's not use exec() to invoke PHPCS. Something like this should do the trick:

PHP_CodeSniffer::setConfigData('installed_paths', $value);

I manually checked this patch, and indeed it updates vendor/squizlabs/php_codesniffer/CodeSniffer.conf

Oh yeah good point. I'm not sure I've done this correctly, but at least the root composer.lock file shows the right version.

Edit: deleted. No matter what I say it will be twisted into being an insult within two comments. There was a time when we had kind people who were trying to work together and not riding on words even when those words were extremely, unduly harsh but those days are long gone. So now you lost a valuable comment and I probably won't comment on any non-migrate issue again for a few months. Good riddance, I guess.

This change is about maintaining coding standards, not documentation. Yes, documentation can be bad, but the solution is to fix docs, not remove them. I know that pushing doc changes can be hard, but that does not mean we simply give up and don't document anymore.

Edit: deleted. No matter what I say it will be twisted into being an insult within two comments.

I think that is insulting to people that contributed lots of documentation and the documentation maintainers that put in countless hours to make docs better.

Let me try to explain where chx was coming from.

He was told to use --standard=Drupal, which has rules for things like: Every function comment needs to have a documentation attached. From his point of view, these automatic rules, makes people lazy instead of thinking.

Just to be clear, so far the core rules file, doesn't include this rule, so the problem chx describes is not directly an issue as of now.

From my point of view there is certainly some truth behind it, but on the other hand this is my opinion:
Even if this lazyness would be triggered by tools, the actual underlying problem is a cultural one. This cultural is hard to fix, and we know we have this problem already. This is not at all new or introduced by this patch.

On the longrun though the win of the MUCH BETTER friendlyness for newcomers outweights this risk.

+++ b/core/composer.json
@@ -36,10 +36,12 @@
+        "squizlabs/php_codesniffer": ">=2.5.1",

this line can be removed, coder already requires PHPCS.

Feel free anyone to jump in and fix the review from @klausi :)

Cool, but we forgot to remove Coder's test files, same for PHPCS. I'm sweating a bit right now that there might be a Coder test file that does weird stuff I forgot about and should not end up on a Drupal production site.

I don't like to put even more crap on people's production sites, should we postpone this until #2745355: Use "composer install --no-dev" to create tagged core packages is done?

Hm, why do we use the post-autoload-dump composer event here? I think we only need to write the config when Coder is first installed, so post-package-install should be used?

OK, so we won't change the Composer event.

We have not removed Coder and PHPCS test files yet, right?

Otherwise looks good.

I suppose we would just add

     'drupal/coder' => ['Test'],

squizlabs/php_codesniffer does not seem to ship with any tests.

Do we care about Docs directories?

Created an issue in Coder to remove the test files from the package: #2783285: Declare files which should not be packaged for production.

This uses the recommended approach of adding a .gitattributes file that marks the files that should not be included in a package intended for production environments.

The obvious problem here is that of course Coder 8.x-2.7 does not include this patch. Even the latest version 8.x-2.8 doesn't include it. To do this by the book we should get a 8.x-2.9 release out the door, and make core comply with it. That would take a bit of effort.

I think the best solution would be to make a 8.x-2.7.1 release that is just 2.7 + the .gitattributes so we can use that to move this issue forward and not be blocked on anything else.

I'll roll an experimental patch that includes this approach.

Here is a proof of concept. I've made a fork of Coder that has a 8.2.8.1 release which is version 8.2.8 + the patch from #2783285: Declare files which should not be packaged for production.

If I install the composer dependencies now, I get a version of Coder that does not have the test files in it.

I really don't think we should say 8.2.* we need to pin to a specific version. This is not the same as other dependencies I don't think we want to semver coding standards. This has been discussed before in this issue. Core is not compliant with 8.2.7 or 8.2.6 now and it probably will not be compliant with 8.2.9 out of the box.

Thank you @Mile23!

A random failure in a test related with random stuff. That's not too bad :)

Coder 8.2.9 has been released :)

Just checked and the patch still applies without problems. Last test result from September 9 was green so this was technically still RTBC.

Some things might have changed in the meantime so I've started a retest to be sure. Awaiting those results I'm tentatively setting the state back to RTBC.

We still need to resolve #2745355: Use "composer install --no-dev" to create tagged core packages before we do this.

Rerolled, otherwise patch is unchanged.

This is not yet a blocker here I think. I've just tried it when I rerolled and Coder still gets installed correctly. This will only become a problem once we define https://packages.drupal.org/8 as the repository in composer.json. That is not in scope of this issue.

If that would get added in another issue before this gets in, then we'll have to deal with it indeed.

OK I think I am allowed to RTBC this, none of this work is actually mine. I have only done reroll duties and provided a PoC patch in #61 so we could test the exclusion of test files. This code did not end up in the current patch.

Testbot failure, resetting status due to https://www.drupal.org/node/2828045

Back to RTBC.

I think before we commit this we need a warning on the status report about the presence of dev dependencies - so people who have production sites can detect this and clean up.

I don't think there is an easy way to detect installed dev dependencies because composer does not mark them as such. How do you know if a Drupal module uses Mink for crawling some other site? So Mink would not be a dev dependency in that case? You could argue that PHPUnit is surely just meant for development, so the most primitive check could be class_exists('\PHPUnit_Framework_TestCase') ...

Anyway, that problem requires more discussion and will probably take some time to get right.

What do we do in the meantime? Put the current version of Coder that should be used in some other text file?

I had a look and indeed it doesn't seem that Composer records whether or not it has included dev dependencies in the lock file. This seems like valuable information to have. Maybe we could propose to include this?

Also it's true: Composer doesn't tell you whether you used --dev or --no-dev to install, because your deployment process should be in charge of that.

That is very true.

Maybe we need a follow-up to do a best guess for the status report, with some docs on what it means.

Yes we do. I remember discussing this around the time of the release but at the moment I don't see that an issue was generated. But this issue would not be postponed by it.

#2830880: Warn site admins when composer dev dependencies are installed inside of docroot

Just a fuzz, no changes, back to RTBC, still applies. Diffed the patches to double check.

More fuzz

+++ b/core/composer.json
@@ -38,6 +38,7 @@
+        "drupal/coder": "8.2.8",

Can we use drupal composer endpoint here? i.e. "drupal/coder":"2.8". As per #84 you have to mention the mapping in composer.json to move it to correct place.

Tagging this issue.

Patch looks good but the most import bit is missing ;-)

1. +++ b/composer.lock
   @@ -3037,6 +3037,42 @@
   +            "name": "drupal/coder",
   

   composer.json entry missing for drupal/coder.

I think, we also need a change record for this.

The latest change looks good to me and I verified it is identical with upstream. Thanks for the handy link, @Mile23!

The timestamp changes are a bit unfortunate but we are going to run into them at some point anyway. And because we are always behind on package updates it's not super trivial to do a no-op update which just contains the timestamp changes in a separate patch. So in my opinion it should be fine to get this in in its current form. It's actually probably better to have this change in a fairly small-scoped issue than in something like a Symfony update or so...

Since we don't require the composer/composer package (and shouldn't), I've copy-pasted the hash computation from Composer to our test.

@alexpott picked the same solution and produced almost the same code in #2843259-2: Drupal\Tests\ComposerIntegrationTest breaks when composer.lock generated with composer version 1.3 and higher. Probably best to just get that one in first!

Hmm... I think either would be fine to re-roll depending on which gets in first, TBH. I think the rate of random fails has decreased by now, so setting back to RTBC.

OK, will need a re-roll now that that is in.

This is apparently blocking a coding standards improvement for 8.3.0.

@Mile23, the bulk update is automated. Per our latest policy, most issues should be committed to 8.4.x first during alpha, beta, or RC and only afterward considered for backport to 8.3.x, rather than targeting issues against 8.3.x ahead of time. This issue is a strategic priority though so I can't imagine that we would not backport it. In general we shouldn't move too many issues back to 8.3.x, but for this one it's okay I think. Thanks!

(P.S.: The coding standards results on DrupalCI are really exciting!)

 git ac https://www.drupal.org/files/issues/2744463_122.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 28622  100 28622    0     0  93345      0 --:--:-- --:--:-- --:--:--  101k
error: patch failed: composer.lock:4
error: composer.lock: patch does not apply

Can we get a re-roll so we can get this into the alpha?

Nice, without all the unrelated composer.lock changes now.

Committed and pushed aaf99e1 to 8.4.x and 5bdc70c to 8.3.x. Thanks!

Since we now have a PSA telling people not to deploy dev dependencies to production this change should have a very very limited impact. Worth mentioning in release notes for developers though.

Yay! I was of two minds about this issue: 1) I love phpcs and coding standards, but 2) ug, yet another dev dependency. But alexpott mentioned that by controlling the coder version from the product side, rather than the testbot side, things make a lot more sense. I agree.

Which of core/phpcs.xml.dist or "Drupal" (via coder) is the correct standard now?

Edit: assuming core/phpcs.xml.dist...

Got it. Obviously they're different-it's why I asked ;-) So phpcs.xml.dist are the officially-vetted rules matching https://www.drupal.org/docs/develop/standards/coding-standards whereas the coder project standard contains everything we're considered. Is that accurate?

"Fixed" is the thing. Cool. Sorry for the seeming-(or literal)inanity.