Remove wikimedia/composer-merge-plugin from BLT

grasmash commented 6 years ago

While this plugin provides crucial functionality, it causes many end user issues.

Using this plugin causes strange interactions with other plugins. For instance, it sometimes causes Composer to download and patch dependencies multiple times in a single update. It also requires us to execute composer update multiple times, which is slow and unintuitive.

In order to remove it, we must ensure that all BLT users somehow have all of this config. We also must have a method of updating this configuration for BLT users that does not require direct manipulation of their composer.json files.

{
  "repositories": {
    "drupal": {
      "type": "composer",
      "url": "https://packages.drupal.org/8"
    }
  },
  "require": {
    "drupal/core": "^8.5.0",
    "drupal/config_split": "^1.0.0"
  },
  "require-dev": {
    "behat/behat": ">=3.1 <3.4",
    "behat/mink": "~1.7",
    "behat/mink-selenium2-driver": "^1.3.1",
    "bex/behat-screenshot": "^1.2",
    "drupal/drupal-extension": "~3.2",
    "drupal-composer/drupal-scaffold": "^2.1.0",
    "jarnaiz/behat-junit-formatter": "^1.3.2",
    "se/selenium-server-standalone": "^2.53",
    "jakoch/phantomjs-installer":   "2.1.1-p07",
    "dmore/behat-chrome-extension": "^1.0.0",
    "mikey179/vfsStream": "~1.2",
    "sensiolabs-de/deprecation-detector": "dev-master"
  },
  "autoload-dev": {
    "psr-4": {
      "Drupal\\Tests\\PHPUnit\\": "tests/phpunit/src/"
    }
  },
  "autoload": {
    "psr-4": {
      "Acquia\\Blt\\Custom\\": "src/"
    }
  },
  "extra": {
    "composer-exit-on-patch-failure": true,
    "drupal-scaffold": {
      "initial": {
        "sites/default/default.services.yml": "sites/default/services.yml",
        "sites/default/default.settings.php": "sites/default/settings.php"
      }
    },
    "enable-patching": true,
    "patches": {
      "drupal/core": {
        "Clear Twig caches on deploys": "https://www.drupal.org/files/issues/2752961-130.patch"
      }
    }
  },
  "scripts": {
    "blt-alias": "blt blt:init:shell-alias -y --ansi",
    "drupal-scaffold": "DrupalComposer\\DrupalScaffold\\Plugin::scaffold",
    "nuke": [
      "rm -rf vendor composer.lock docroot/core docroot/modules/contrib docroot/profiles/contrib docroot/themes/contrib",
      "@composer clearcache --ansi",
      "@composer install --ansi"
    ],
    "install-phantomjs": [
      "rm -rf vendor/bin/phantomjs",
      "PhantomInstaller\\Installer::installPhantomJS"
    ]
  }

We can replace the require and require-dev arrays by moving require dependencies into BLT's own composer.json and moving all require-dev packages into a metapackage and then having users require only that one package in their own require-dev. This can be in the default composer.json in https://github.com/acquia/blt-project.

repositories and can probably be moved into the https://github.com/acquia/blt-project default template since it likely won't need to be updated often, if ever, in future.

scripts can be moved into PHP classes and made available commands via the Composer API. They could also just be made into independent PHP scripts.

We need to see if there's a way to use the Composer API to dynamically provide the other config, like autoload and some of the extra config. Worst case, we can use validation hooks and throw warnings when users are missing required config in their composer.json. Users can then correct the issues themselves.

anavarre commented 6 years ago

It also requires us to execute composer update multiple times, which is slow and unintuitive.

I can totally relate to that as this is what just happened to me upon trying to update Lightning to 3.1.2 (got Pipelines tests failing due to the features module missing, among other things).

christopher-hopper commented 6 years ago

One suggestion: perhaps acquia/blt:^9.0 uses acquia/blt-require-dev:^9.0

Could be a good idea. Something to consider.

grasmash commented 6 years ago

See https://github.com/grasmash/bolt/tree/remove-merge-plugin for work in progress.

danepowell commented 6 years ago

Good riddance. Thanks for the PR, let us know if there's anything we can do to help accelerate this!

grasmash commented 5 years ago

Anything you can do would be helpful! Please take the branch and run with it. I'm not user when I'll have time to continue work but I feel it is very important.

grasmash commented 5 years ago

The biggest challenge is this part:

We need to see if there's a way to use the Composer API to dynamically provide the other config, like autoload and some of the extra config.

I'm not sure how to do it and need some help with discovery.

grasmash commented 5 years ago

Branch has been rebased.

Some notable changes in https://github.com/acquia/blt/pull/2878:

acquia/blt-project is now a subtree split of acquia/blt, stored in sub-tree-splits/blt-project. The new architecture requires a closer relationship between the acquia/blt and acquia/blt-project libraries, and keeping them both in the same repo facilitates development and testing of that relationship
projects formerly in composer.suggested.json are now in acquia/blt-project's composer.json. This allows users to inherit them by default when they create a new project, but does not make them a hard dependency of BLT.
require dependencies from composer.required.json libraries are mostly in BLT's own composer.json. Some are no longer required.
require-dev dependencies from composer.required.json have been moved into a new subtree split of BLT, acquia/blt-require-dev. This lives in subtree-splits/blt-require-dev given that it also has an implicit relationship with BLT's own code.
The blt-alias composer script has been removed. The same function can be invoked with blt blt:init:shell-alias -y --ansi
The install-phantomjs composer script has been removed, effectively removing PhantomJS support.
The nuke composer script has been moved to a composer command supplied by the BLT plugin. However, it will now only function if BLT is installed already. Consider creating a separate drupal/composer-nuke project that can be installed globally as an altervative.

grasmash commented 5 years ago

I'm starting to question whether keeping require-dev dependencies out of prod is worth the additional complexity of introducing a new metapackage subtree.

How bad would it be to add these as hard (production) dependencies to BLT?

        "behat/behat": ">=3.1 <3.4",
        "bex/behat-screenshot": "^1.2",
        "drupal/drupal-extension": "~3.2",
        "jarnaiz/behat-junit-formatter": "^1.3.2",
        "se/selenium-server-standalone": "^2.53",
        "dmore/behat-chrome-extension": "^1.0.0",
        "sensiolabs-de/deprecation-detector": "dev-master",
        "webflo/drupal-core-require-dev": "^8.6.1"

It would be much less effort the maintain and use BLT if we made this concession.

@arknoll @ba66e77 @geerlingguy @danepowell @jrbeeman @balsama opinions?

geerlingguy commented 5 years ago

Honestly I'd rather those be top-level requirements exposed to the end user more.

Also there are arguments to be made for Behat at least to be available in prod environments.

danepowell commented 5 years ago

I do think it makes sense to have those as "production" requirements, for one simple reason: so that customers can run automated tests not just in Pipelines/Travis, but also in CDEs or in a dev/stage environment. Since we follow a build-once-deploy-everywhere strategy, if you want to be able to run tests anywhere other than CI, then these need to be full dependencies.

On the other hand, I could see enterprise security departments bristling (rightly or wrongly) at the thought of "development" dependencies existing in production. I don't know if we should make concessions for that or not.

balsama commented 5 years ago

I could see enterprise security departments bristling (rightly or wrongly) at the thought of "development" dependencies existing in production

I think they would be making a good point. SA-CORE-2017-001 included disclosing a vulnerability in PHPUnit which is in webflo/drupal-core-require-dev.

This is probably mitigated somewhat (completely?) since we know that BLT is installing these dependencies outside of docroot.

danepowell commented 5 years ago

Yeah... I think the key is to focus on mitigation. Because once we open the can of worms that is maintaining separate build artifacts for separate build environments, things will get out of hand quickly.

Even today, there are all sorts of things that are only used in a development environment and not in prod (dblog, devel, dummy user account generators, etc...). And we don't have any way to completely remove those in prod, we just rely on the fact that they aren't turned on.

christopher-hopper commented 5 years ago

Representing the interests of "enterprise security departments", who I deal with a lot, I'm here to bristle. Rightly or wrongly Enterprise are looking to Drupal as an Enterprise level CMS, so we try not to make architectural decisions that can be used to argue that Drupal is not ready as an Enterprise grade solution.

I agree, that sometimes Enterprise security requirements seem way OTT. There's often method though to the madness and if you play ball, you get to do some really cool stuff.

My non-BLT projects do not include "drupal/devel" in builds that go to Production. My non-BLT projects remove all tarball test fixtures, README.txt, INSTALL*.txt, etc. from builds, as these regularly come up in Enterprise PenTest reports due to the disclosures sometimes contained inside.

I would like my BLT projects to do the same, without the need to fork BLT commands, or patch around BLT.

BLT is a fantastic accelerator and tool for Drupal teams. BLT is opinionated, and that's okay when BLT enforces best practice on teams.

is BLT = Drupal best practice?

or

is BLT = Drupal made easy?

It doesn't have to be either / or. Surely we can do both. Can BLT make Enterprise grade security and best practice easy and fast for teams to adopt?

typhonius commented 5 years ago

I have to agree with @christopher-hopper here as the clients that we deal with are typically both extremely risk averse and security conscious (and rightly so). If we want to continue to extend BLT/Drupal's foothold in enterprise, then we need to define our best practices with risk in mind and not leave any opening for the good work that's been done to this point to be undermined.

I won't be able to truly justify to a client why development packages are installed on their production instance and that's a potential problem.

grasmash commented 5 years ago

Thanks @christopher-hopper and @typhonius for expressing your opinion. You've encouraged me to push forward and solve the issue with the metapackage. It's working now, we won't need to concede the best practice.

grasmash commented 5 years ago

This change should be considered backwards incompatible due to the update steps. I'd release this in a 10.0.0 and make it correspond with dropping support for PHP 5.6.

One issue I haven't resolved: adding drupal/core (or anything that requires it) to BLT's own composer.json causes these warnings during Composer operations:

Could not scan for classes inside "/tmp/blt-sandbox-instance/vendor/drupal/core/lib/Drupal.php" which does not appear to be a file nor a folder
Could not scan for classes inside "/tmp/blt-sandbox-instance/vendor/drupal/core/lib/Drupal/Component/Utility/Timer.php" which does not appear to be a file nor a folder
Could not scan for classes inside "/tmp/blt-sandbox-instance/vendor/drupal/core/lib/Drupal/Component/Utility/Unicode.php" which does not appear to be a file nor a folder
Could not scan for classes inside "/tmp/blt-sandbox-instance/vendor/drupal/core/lib/Drupal/Core/Database/Database.php" which does not appear to be a file nor a folder
Could not scan for classes inside "/tmp/blt-sandbox-instance/vendor/drupal/core/lib/Drupal/Core/DrupalKernel.php" which does not appear to be a file nor a folder
Could not scan for classes inside "/tmp/blt-sandbox-instance/vendor/drupal/core/lib/Drupal/Core/DrupalKernelInterface.php" which does not appear to be a file nor a folder
Could not scan for classes inside "/tmp/blt-sandbox-instance/vendor/drupal/core/lib/Drupal/Core/Site/Settings.php" which does not appear to be a file nor a folder

All tests still pass and it seems to have no functional impact, but it's very annoying and I cannot figure out how to resolve it.

anavarre commented 5 years ago

I'd release this in a 10.0.0 and make it correspond with dropping support for PHP 5.6.

That seems like a great idea.

danepowell commented 5 years ago

Very pleased to call this closed now in the 10.x branch, but with a follow-up issue for the class warning. #3148

acquia / blt

Remove wikimedia/composer-merge-plugin from BLT #2744