Special handling for composer-based workflows

[jhedstrom]

This is the corresponding PR to phase2/generator-gadget#66

[grayside]

I like the detection for a make path to see if composer should be used.
I didn't realize you could run composer update without a lockfile. How does it work differently than composer install?

Relates to phase2/generator-gadget#66

[jhedstrom]

We need to manually run composer drupal-scaffold here too, or go figure out why Drupal Scaffold removed it.

[arithmetric]

This is looking good. I'd like to see some test coverage for the composer-based workflow route. Do you have an example configuration?

[jhedstrom]

I've added tests (which includes an example configuration). I also re-worked some of the d7 example/test assets to reflect that .make files will not be the default in D8.

I updated the install task to include composer drupal-scaffold as mentioned above.

[grayside]

Dropping test coverage for Drush Make for D8 implies this should be merged into pre-v1. I'm okay with that.

[grayside]

Overall in favor here, though as mentioned I do think it should be targeted at the 1.0 release. I'm comfortable with the docs as a follow-up but 1.0 blocker if that helps.

This does make the particular details of the (generated) composer.json a bigger part of how grunt-drupal-tasks works, as we're relying on some fairly elaborate, non-standard code being triggered by composer management in a way we never did with Drush. The 1.0 docs already focus on Gadget as the way to get started, but this change really makes Gadget almost a requirement.

Not sure what the implications are of that, if any.

[mike-potter]

I'd like to see this sooner rather than later. The impact on non-composer builds is really minimal and the changes to only run drushmake if there is a makefile in the src just makes good sense regardless.

I don't think we are really relying on a lot of "non-standard code" here. The only tricky part I have run into with composer is the drupal-scaffolding stuff which is more of a drupal issue than a composer issue. I've actually created test files without Gadget, so even that isn't really required.

This PR is currently a blocker for both a client project as well as our own P2 distro, so I'll help as I can with whatever blockers there are in getting this committed so we can get more people testing composer-based builds.

[grayside]

Agreed on the significance. Composer drupal-scaffold is what I was referring to, and it makes a key part of building gdt-based projects reliant on gadget. That's not a blocker, more of a philosophical shift which perhaps needs our Readme updated.

I'd still like this to be a 1.0 change if possible, will cut down our road map further.

[mike-potter]

I'm investigating the drupal-scafoold stuff more today. I'm not sure it requires Gadget actually. The Lightning "pure-composer" build process is working fine without Gadget, so I think there might be an alternative way to handle this. Before Jonathan added the composer:drupal-scaffold I had a site building with Lightning and all the Drupal stuff and need to figure out a way to reproduce what I had working.

I'll leave the version issues to you and others since I haven't been involved in the road mapping.

[grayside]

Interesting point about what drupal-scaffold really gets us. As I understand, it's mostly the particular code repo structure, which is part of GDT function.

[jhedstrom]

I had a site building with Lightning

Doesn't Lightning's composer.json call drupal-scaffold?

As I understand, it's mostly the particular code repo structure, which is part of GDT function.

It doesn't actually scaffold so much as download all the root files from core that sit alongside the core directory.

[mike-potter]

So I did some testing of this outside of GDT with just pure composer. Using a composer.json that has this:
"scripts": {
"drupal-scaffold": "DrupalComposer\DrupalScaffold\Plugin::scaffold"
},
"require": {
"roave/security-advisories": "dev-master",
"composer/installers": "^1.0",
"drupal-composer/drupal-scaffold": "^2.0.1",
"cweagans/composer-patches": "1.5.0",
"drupal/lightning": "8.1.*"
}
...

1. Starting clean (no lock file, no build dir, no vendor dir).

2. Neither composer install nor composer update installed the drupal scaffold (e.g., the index.php file).

3. Ran composer drupal-scaffold to force the drupal scaffolding to be created.

4. Once Adding .travis.yml for Travis CI integration. #3 was done, deleting the build dir and composer.lock file, both composer install and composer update caused drupal-scaffold script to run automatically

So, composer drupal-scaffold only appears to be needed the first time. Rather than checking for the composer.lock file, I recommend checking for the build/html/index.php file. That will indicate whether a composer drupal-scaffold is needed or not. And composer drupal-scaffold should run after the composer install or composer update. This is similar to what we already check in the GDT scaffold task.

I don't recommend using composer update at all. That should only be used when we want dependencies updated. On a fresh site there is no difference seen between composer install and composer update and once a site is built it seems like using composer install is the correct thing to do.

For example, even if our composer.json file is pointing to Lighting 1.1.00-rc6, doing a composer update could cause newer untested modules to be downloaded because Lightning does not pin modules to specific releases and the Lightning composer.lock file is ignored when used in a child dependency. Thus, we don't want to be using composer update unless we are sure about what we are doing.

[jhedstrom]

So, composer drupal-scaffold only appears to be needed the first time

That's only true if we commit the files it adds. Otherwise, any subsequent developer would need that to run. It also needs to run when core version is upgraded in case those files have changed (which I think is why it is set to run automatically on a composer update).

[jhedstrom]

The docs encourage committing the scaffolding files, in which case it would only have to run the first time, or on intentional upgrades of core.

[mike-potter]

So scaffold stuff is resolved.

What is left for this to get merged? Doesn't impact any existing projects, only composer-based stuff. We really need to at least get the "if (grunt.config.get('config.srcPaths.make'))..." condition committed since that is a current blocker for projects that do not have any drush make file.

[grayside]

• I've noted the path to the Drupal composer repository should be updated to make sure uptime stays good for our tests
• Needs documentation updates based on the pre-v1.0 branch changes to docs
• Needs manual merge into the pre-v1.0 branch

[jhedstrom]

@grayside I've done the first 2, I have not done the manual merge. I've also updated the corresponding gadget pr with the composer repository change.

[arithmetric]

Thanks @jhedstrom, I'm looking forward to getting this in.

When testing this today, I found that the autoload.php file generated in the docroot references an autoload.php file in the vendor directory in the project's root directory. This is fine for the normal build, but it appears that the version produced by grunt package also references this file outside the docroot. Is it possible for the Composer dependencies (without dev) to be included in the package output?

I agree with @grayside that this should go into the v1.0.0-pre branch. I'm willing to help with that merge.

[arithmetric]

This branch has composer-workflow merged into v1.0.0: https://github.com/phase2/grunt-drupal-tasks/tree/v1.0.0-pre-composer-workflow

Tests are looking good: https://travis-ci.org/phase2/grunt-drupal-tasks/builds/137954782

[jhedstrom]

Is it possible for the Composer dependencies (without dev) to be included in the package output?

This is going to take a bit of work I think. In the pre-composer version, all of the project's production dependencies were able to be packaged since they lived in build/html. With composer, it is considered a best practice to place vendor outside of the docroot, so all of Drupal and other php libraries reside in ./vendor as opposed to build/html/vendor (where they would be if Drupal were downloaded via a tarball).

[arithmetric]

@jhedstrom I can appreciate wanting to keep vendor outside the docroot. I'd like to find a solution that allows the package output to be self-contained and work relatively easily in common environments (especially where the docroot is at /var/www/html).

I ran into this issue when trying to mount the package output at /var/www/html in a standard Drupal Docker container. Since the container did not have access to the vendor directory outside the package output, Drupal failed to load.

Given the docroot living at /var/www/html, I could see the vendor contents placed at /var/www/vendor (but not at /var/vendor, two levels up, as would match the build/html environment).

So a few questions:

• Where should the vendor directory live in a production deployment?
• Does the package task need to be altered to generate the autoload link to this location? (Should it re-run composer?)
• Is it preferable to remove dev dependencies from the package output? How feasible is this?

[mike-potter]

I just ran into this with a client that wanted to use our Octane repo example for Lighting/composer. We needed to change the "installer-paths" to be relative to the current dir and get the vendor directory created in the current dir so we could put the entire current dir into a repo for Pantheon.

So I think having the vendor dir as close to the docroot as possible makes sense. Projects that need to push an entire repo to a hosting provider will need the vendor dir in the docroot. I think that means the production server probably needs vendor in the docroot. For GDT it seems like maybe we could add a configuration to handle it in different places maybe?

[grayside]

Package can whitelist/blacklist different directories, as well as move them around. In practice, this means if the docroot knows to look one level up, we can have our composer and package configuration make that so. However, that gets dicey with Pantheon, which will probably require the vendor/ directory to be inside the docroot.

This also doesn't address the point about development dependencies. While I don't see a harm from including PHPCS, PHPMD, Behat, and so on, it certainly bloats the package size a bit.

One thing we could do, after running the package command to shape (copied) release package, we could use the tools installed in the build environment to strip the dev dependencies. It looks like composer remove --update-no-dev is something worth playing with. One concern I have is the implication that it runs composer update, we'd want to find a compromise of leveraging the composer.lock but just stripping out the dev deps. Worse to worse, we rm the vendor directory and run composer install?

[arithmetric]

As @grayside mentioned, I see that the package task already can be configured to put the build output in paths to preserve the expected relationship.

The following changes in Gruntconfig.json will include the vendor directory under the package path and the docroot output under build/html in the package path:

   "packages": {
     "srcFiles": ["!sites/*/files/**", "!xmlrpc.php", "!modules/php/*"],
-    "projFiles": ["README*", "bin/**", "hooks/**", "src/*.make"]
+    "projFiles": ["README*", "bin/**", "hooks/**", "src/*.make", "vendor/**"],
+    "dest": {
+      "docroot": "/build/html",
+      "devResources": ""
+    }
   },

By symlinking /var/www/html to the package's build/html directory, I was able to get Drupal to load.

As @mike-potter said, this will not work in environments where you cannot deploy outside of the docroot. To support those environments, the paths in the autoloader files generated by Composer will need to be updated to match the structure of the package output.

[arithmetric]

With this configuration change, I'm going to merge this into the v1.0.0-pre branch and will be pushing out a v1 alpha soon.

I've created issues #288 and #289 for follow ups on the Composer dependency location and excluding dev dependencies.

[arithmetric]

This is merged to master, and available as version 1.0.0-alpha1.

It is not tagged as the latest release, so install it by version: npm i [email protected]

[grayside]

For use in projects, that probably means npm i --save [email protected]?