Problem/Motivation

Now that we're well on our way to managing coding standards in a reproducible way: #2571965: [meta] Fix coding standards in core

...let's just add the tools to core's composer.json dev requirements.

That way we don't have to instruct people how to install them beyond saying "run composer install --dev"

Proposed resolution

Modify core/composer.json to add drupal/coder and squizlabs/php_codesniffer as a dev requirement.

Add an install script to composer.json which adds coder's code_sniffer as an installed_path.

Remaining tasks

Update documentation such as https://www.drupal.org/node/1419988 and https://www.drupal.org/node/1587138

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#130 2744463_130.patch8.38 KBMile23
#122 2744463_122.patch27.95 KBMile23
#111 interdiff.txt1.94 KBMile23
#111 2744463_111.patch29.89 KBMile23
#109 interdiff.txt20.68 KBMile23
#109 2744463_109.patch27.95 KBMile23
#106 2744463_105.patch8.71 KBMile23
#100 2744463-100.patch8.98 KBjoelpittet
#98 2744463-98.patch8.98 KBjoelpittet
#10 2744463_10.patch8.6 KBMile23
#13 2744463_13.patch8.6 KBMile23
#13 interdiff.txt657 bytesMile23
#17 2744463_17.patch8.6 KBMile23
#17 interdiff.txt460 bytesMile23
#27 2744463_27.patch8.6 KBMile23
#27 interdiff.txt460 bytesMile23
#40 2744463-40.patch8.72 KBdawehner
#40 interdiff.txt1.31 KBdawehner
#42 2744463-42.patch8.72 KBdawehner
#42 interdiff.txt461 bytesdawehner
#50 2744463_50.patch8.58 KBMile23
#50 interdiff.txt400 bytesMile23
#54 2744463_54.patch8.57 KBMile23
#61 2744463-61.patch9.6 KBpfrenssen
#61 interdiff.txt3.28 KBpfrenssen
#62 2744463_62.patch8.95 KBMile23
#62 interdiff.txt1.05 KBMile23
#64 2744463_64.patch8.95 KBMile23
#64 interdiff.txt1.72 KBMile23
#79 2744463-79.patch8.96 KBpfrenssen
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Mile23 created an issue. See original summary.

dawehner’s picture

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

pfrenssen’s picture

I am in favor of this too.

alexpott’s picture

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.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

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

RoSk0’s picture

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.

dawehner’s picture

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

Mile23’s picture

Status: Active » Needs review
Issue tags: +composer
FileSize
8.6 KB

phpunit is pretty much required by simpletest module, so it should not actually be --dev. But it has that big classmap and is probably a security risk so it should be in --dev. Make up your mind, Drupal! :-)

Here's a patch. We explicitly add both drupal/coder and squizlabs/php_codesniffer as dev requirements to core/composer.json. We add a post-autoload-dump script to both root and core composer.json files (ideally it would only be added to core, since that's where we're making the requirement, but scripts will only run from root for normal dev operation). The script is a static method that checks if both packages are present, and if they are, configures phpcs to use the coder_sniffer directory to find sniffs.

The test:

  • Apply the patch.
  • Run composer install.
  • Composer will add drupal/coder and phpcs.
  • $ cd core
  • $ ../vendor/bin/phpcs -i shows you that the Drupal standard is installed.
  • $ ../vendor/bin/phpcs -p -s
  • phpcs runs normally.

I chose post-autoload-dump because it's an easy place to trigger during testing. We could move this to another event, so maybe someone knows a better one.

I experimented with calling \PHP_CodeSniffer::setConfigData() directly to avoid an exec() call, but there's clearly some part of the setup I don't understand. The settings didn't stick.

RoSk0’s picture

+++ 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.

alexpott’s picture

+++ 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.

Mile23’s picture

@alexpott: So who's going to keep their eye on the coder version number? It would suck to have a lot of improvements in coder that we don't use in core because no one updated the composer file.

Anyway. Pinning coder to 8.2.7 because that's what composer installed when I asked it to, and running it only gave me two Drupal.Commenting.FunctionComment.ThrowsComment errors.

Also bumping phpcs to >=2.5.1 since that's what coder needs. In other circumstances we might not need to declare a requirement for phpcs, but since we have code that installs our sniffs we should declare it.

Mile23’s picture

Still applies, still works. :-)

tstoeckler’s picture

Status: Needs review » Needs work

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 ?!

tstoeckler’s picture

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...

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.6 KB
460 bytes

drupal/coder is now ~8.2

tstoeckler’s picture

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

alexpott’s picture

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.

alexpott’s picture

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.

tstoeckler’s picture

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.

alexpott’s picture

@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.

tstoeckler’s picture

OK, so ~8.2.7 then?

Mile23’s picture

Take your pick. :-) #13 specifies 8.2.7, and #17 specifies ~8.2. They're the same otherwise. You'll note that the lock file in #17 is unchanged from #13 because they both resolve to 8.2.7 at this time.

Also, core currently doesn't pass 8.2.7 because of #2747073: Fix Drupal CS regressions for Coder 8.2.8 but that's easy to fix.

tstoeckler’s picture

Status: Needs review » Needs work

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

tstoeckler’s picture

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.

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.6 KB
460 bytes

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.

Based on this, I'd say we should specify 8.2.*

Obviously we can revisit this as coder changes versioning or whatever else.

dawehner’s picture

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.

Mile23’s picture

So yah, having some flexibility would be good to allow more compatibility latitude for external projects like drupal-project.

The composer.lock file locks down drupal/drupal in the core repo, and something like drupal-project will ignore that. If drupal/core has a hard requirement on a specific version, then we might make dependency issues for other projects. However, I doubt that's the case, and we can adjust as needed, either as a change to other projects or core.

I just don't want to have to keep typing ./vendor/bin/phpcs --config-set installed_paths /ridiculously/long/path/to/a/thing every time I review a CS issue. :-)

We have a patch for each of the options. Someone decide. :-)

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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 :-)

dawehner’s picture

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?

tstoeckler’s picture

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

alexpott’s picture

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.

alexpott’s picture

klausi’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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" />
tstoeckler’s picture

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.)

klausi’s picture

Status: Needs review » Needs work

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

PHP_CodeSniffer::setConfigData('installed_paths', $value);
Mile23’s picture

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

I tried that and couldn't get it to work. You're welcome to try, please. :-)

Mile23’s picture

Coder now has an 8.2.8 version which kills us here. :-)

I was getting so many errors I just stopped it before it finished.

Reverting back to 8.2.7 leaves only the few regressions we already knew about: #2747073: Fix Drupal CS regressions for Coder 8.2.8 plus a couple Drupal.Classes.UnusedUseStatement.UnusedUse that don't have an issue yet.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.72 KB
1.31 KB

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

Mile23’s picture

Status: Needs review » Needs work
  1. +++ b/core/composer.json
    @@ -36,10 +36,12 @@
    +        "drupal/coder": "8.2.*",
    

    We really should set it to 8.2.7 for now so we can at some point say we're finished with this round of fixes. :-)

    Make a follow-up to add the * back?

  2. +++ b/core/lib/Drupal/Core/Composer/Composer.php
    @@ -136,6 +137,28 @@ public static function ensureHtaccess(Event $event) {
    +      PHP_CodeSniffer::setConfigData('installed_paths', $vendor_dir . '/drupal/coder/coder_sniffer');
    

    Weird. That didn't work for me. Call it user error. Works now in my local installation.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.72 KB
461 bytes

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.

chx’s picture

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.

klausi’s picture

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.

chx’s picture

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

klausi’s picture

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.

dawehner’s picture

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.

klausi’s picture

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

this line can be removed, coder already requires PHPCS.

dawehner’s picture

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

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.58 KB
400 bytes

Not really sure what @chx was saying because he apparently edited those comments.

@dawehner: From his point of view, these automatic rules, makes people lazy instead of thinking.

If you have a preconfigured tool to do the CS review and you use it, you can spend your energy on things which require it - such as what a docblock says - rather than things which shouldn't require it, such as whether the file ends with a newline or something like that.

If you're a maintainer, this makes it easier to say what's wrong with a patch from CS perspective, and gives the contributor a way to reproduce the review and maybe even find problems the maintainer didn't.

Basically: If you can achieve the same goal while being lazy, then you are one of life's winners.

@klausi: this line can be removed, coder already requires PHPCS.

The thought was that since our installer script needs both, we should require it explicitly, but it's not a deal breaker. :-)

klausi’s picture

Status: Needs review » Needs work

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?

klausi’s picture

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?

Mile23’s picture

Cool, but we forgot to remove Coder's test files, same for PHPCS.

Yah, so that means we also have to depend on phpcs explicitly, if we're going to dig out the tests.

Hm, why do we use the post-autoload-dump composer event here?

Originally just because it was easy to debug. :-)

If we called exec() then it would be fine to change to another event.

But since we now call PHP_CodeSniffer::setConfigData() the script has to be able to autoload that class.

Mile23’s picture

Version: 8.2.x-dev » 8.3.x-dev
FileSize
8.57 KB

Bumping up to 8.3.x.

Mile23’s picture

Status: Needs work » Needs review
klausi’s picture

Status: Needs review » Needs work

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

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

Otherwise looks good.

Eric_A’s picture

Title: Add phpcs, coder as --dev requirements for core » Add phpcs, coder 8.2.7 as --dev requirements for drupal/core
Eric_A’s picture

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?

pfrenssen’s picture

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.

pfrenssen’s picture

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
9.6 KB
3.28 KB

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.

Mile23’s picture

Title: Add phpcs, coder 8.2.7 as --dev requirements for drupal/core » Add phpcs, coder 8.2.* as --dev requirements for drupal/core
FileSize
8.95 KB
1.05 KB

Well the reason we wanted to hard-code to 8.2.7 is because it would introduce a bunch of regressions to allow 8.2.8+. But that's dealt with in #2747073: Fix Drupal CS regressions for Coder 8.2.8 so core is now good with whatever version we want. There are a couple follow-up issues from that, if anyone wants to dive in. :-)

Future versions of Coder will have .gitattributes, but we should still add a deletion to our Composer script, for current versions.

phpcs has a .gitattributes file which deals with their test directory as discovered in #58: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/.gitattributes

So: This patch adds a dependency for drupal/coder:8.2.*, adds some directories to delete in drupal/coder, and keeps the same install script.

I did a composer update --lock which resulted in no change, so we're good. :-)

alexpott’s picture

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.

Mile23’s picture

OK. Pinned to 8.2.8.

Status: Needs review » Needs work

The last submitted patch, 64: 2744463_64.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review

Unrelated fail. Setting to needs review, re-starting test.

Mile23’s picture

Title: Add phpcs, coder 8.2.* as --dev requirements for drupal/core » Add phpcs, coder 8.2.8 as --dev requirements for drupal/core
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Mile23!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 2744463_64.patch, failed testing.

dawehner’s picture

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

pfrenssen’s picture

Title: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core » Add phpcs, coder 8.2.9 as --dev requirements for drupal/core

Coder 8.2.9 has been released :)

Mile23’s picture

After #2747073: Fix Drupal CS regressions for Coder 8.2.8 we're up to date (more or less) with Coder 8.2.8. So we should stick with that for now. Especially since 8.2.9 gives us regressions: #2803779: Update Coder to 8.2.10 and fix errors from improved sniffs

This issue really should be in 8.2.x but since 8.2 is in RC, we probably won't be adding external dependencies. Any maintainers have an opinion on that?

Mile23’s picture

Title: Add phpcs, coder 8.2.9 as --dev requirements for drupal/core » Add phpcs, coder 8.2.8 as --dev requirements for drupal/core
pfrenssen’s picture

Status: Needs work » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Postponed
Mile23’s picture

Mile23’s picture

Mile23’s picture

Status: Postponed » Needs work
pfrenssen’s picture

Status: Needs work » Needs review
FileSize
8.96 KB

Rerolled, otherwise patch is unchanged.

pfrenssen’s picture

Mile23’s picture

Talked with @Mixologic in IRC about this and he helpfully pointed me to #2802009: Provide support for libraries and other non-drupal extensions (modules, themes etc) which might be a blocker here.

pfrenssen’s picture

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.

Mile23’s picture

Well except that we're telling people to add packages.drupal.org/8 as a repository so they can install modules.

It kind of works now because coder has a packagist entry.

Mile23’s picture

I just tried it.

  • I applied the patch in #79.
  • Added
        "repositories": {
            "drupal": {
                "type": "composer",
                "url": "https://packages.drupal.org/8"
            }
        },
    

    to the root composer.json file.

  • rm -rf vendor
  • rm composer.lock
  • composer update

That yields drupal/coder as a library in vendor/, rather than a module. It still kind of only works by accident until we get #2802009: Provide support for libraries and other non-drupal extensions (modules, themes etc) sorted.

I'd RTBC but it's my patch that got rerolled. :-)

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: 2744463-79.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: 2744463-79.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

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.

klausi’s picture

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?

pfrenssen’s picture

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?

Mile23’s picture

I think before we commit this we need a warning on the status report about the presence of dev dependencies

Is that a concern on this issue? Or is it a concern on #2745355: Use "composer install --no-dev" to create tagged core packages which is already marked fixed?

We already have some dependencies as --dev, and we already have core packaged as --no-dev.

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

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.

pfrenssen’s picture

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.

cilefen’s picture

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.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: 2744463-79.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.98 KB

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: 2744463-98.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.98 KB

More fuzz

jibran’s picture

+++ 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.

Mile23’s picture

Can we use drupal composer endpoint here?

That's kind of the point of #2802009: Provide support for libraries and other non-drupal extensions (modules, themes etc)

The Drupal composer facade imposes type: drupal-module on d.o projects that don't have info.yml files, such as Coder. So there's a special case for coder baked in to the infrastructure, but other projects might not fare so well if we add the d.o facade.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 100: 2744463-100.patch, failed testing.

dimaro’s picture

Issue tags: +Needs reroll

Tagging this issue.

Mile23’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Again we go. :-)

Will core have the tool first? Or will #1266444-55: DrupalCI should return failed for poor code style - php win the race?

Mile23’s picture

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
jibran’s picture

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.

Mile23’s picture

Status: Needs work » Needs review
FileSize
27.95 KB
20.68 KB

composer.json entry missing for drupal/coder.

It's in core/composer.json, since it's a requirement of drupal/core and not drupal/drupal.

But I did neglect one thing... Forgot the merge-dev line in the root composer.json. This is the default for the wikimedia merge plugin, but it's best to be explicit.

Fixed here.

Added change record: https://www.drupal.org/node/2839574

Status: Needs review » Needs work

The last submitted patch, 109: 2744463_109.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
29.89 KB
1.94 KB

That last patch failed ComposerIntegrationTest::testComposerLockHash().

The reason is that Composer 1.3.0 changes the behavior of the lock file, so it no longer has the easy-to-test-for hash, and instead has the much-more-picky content-hash.

This means our test will fail eventually for any circumstance where we generate the lock file with a version of Composer >= 1.3.0, so we have to fix this test anyway.

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

Earlier versions of Composer add both hashes, so they should be present and calculated in the same way if someone uses an older version of Composer to generate the lock file. And thus will pass the test.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 111: 2744463_111.patch, failed testing.

Mile23’s picture

Status: Needs work » Reviewed & tested by the community

I think (hope?) that the test fail is unrelated so I'll re-run.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 111: 2744463_111.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review

Failing tests here are mentioned on #2829040: [meta] Known intermittent, random, and environment-specific test failures

Rather than re-run the test again, so someone can RTBC, so it can fail intermittently, I'll just leave this as NR.

Eric_A’s picture

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!

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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.

Mile23’s picture

Re-running test in #111.

Strictly speaking, having a commit message that says this is about composer 1.3 is probably better than a commit message saying it's about coder.

Mile23’s picture

Status: Reviewed & tested by the community » Postponed

This is postponed on #2843259: Drupal\Tests\ComposerIntegrationTest breaks when composer.lock generated with composer version 1.3 and higher, and in fact all composer-based patches should be as well.

tstoeckler’s picture

Status: Postponed » Needs work

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

Mile23’s picture

Status: Needs work » Needs review
FileSize
27.95 KB

Re-rolled.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: +8.3.0 release notes

This is apparently blocking a coding standards improvement for 8.3.0.

Mile23’s picture

Version: 8.4.x-dev » 8.3.x-dev

Are we really postponing this until 8.4.x when just yesterday we made the testbots do phpcs reports? :-) #2841472: DrupalCI should return failed for poor code style Part II - php

This isssue doesn't make any code changes, and only adds a dev dependency to core. I hope this can be RC eligible.

xjm’s picture

@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!)

Mile23’s picture

Version: 8.3.x-dev » 8.4.x-dev

OK, thanks.

alexpott’s picture

Issue tags: +Needs reroll
 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?

Mile23’s picture

Rerolled for 8.4.x

Mile23’s picture

Status: Reviewed & tested by the community » Needs review
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed aaf99e1 on 8.4.x
    Issue #2744463 by Mile23, dawehner, pfrenssen, joelpittet, tstoeckler,...

  • alexpott committed 5bdc70c on 8.3.x
    Issue #2744463 by Mile23, dawehner, pfrenssen, joelpittet, tstoeckler,...
cilefen’s picture

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.

cilefen’s picture

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

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

Mile23’s picture

core/phpcs.xml.dist is a subset of the Drupal standard. If you look inside the Drupal standard directory of Coder here: http://cgit.drupalcode.org/coder/tree/coder_sniffer/Drupal

...you'll see ruleset.xml, which is the whole thing.

The correct standard is here: https://www.drupal.org/docs/develop/standards

cilefen’s picture

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?

Mile23’s picture

There's a meta here with an overview: #2571965: [meta] Fix coding standards in core

core/phpcs.xml.dist contains all the sniffs that have been fixed.

So if you fix a sniff, you also patch core/phpcs.xml.dist to reflect it.

That way the maintainers don't go insane. :-)

cilefen’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.