Problem/Motivation

This issue creates infrastructure which allows a Composer-based Drupal installation to mimic an archive-download or git-clone based one.

In order to support the build process with Composer, we'll need a plugin that is capable of deploying the various scaffold files and directories that exist in the Drupal git repository to their proper locations.

This plugin will be a very important dependency of the 'drupal/drupal-project-legacy' package. The purpose of the 'drupal/drupal-project-legacy' component is to replicate the current zip file or tarball you download from drupal.org. This allows the Drupal core product to be compatible with the tarball approach, while reducing or eliminating the complications that arise from switching a tarball installation to a Composer-based one. Details: #2982680: Add composer-ready project templates to Drupal core

Proposed resolution

Model the plugin after the community plugin that already exists:
https://github.com/drupal-composer/drupal-scaffold

This plugin should be able to recreate exactly the existing drupal tarballs that we have, and also be configurable/ flexible enough to allow for various kickstart packages to place scaffold files wherever a maintainer may want them. (to support various docroots etc)

This plugin is intended to by usable by distribution maintainers to setup their distros.

Remaining tasks

  1. Determine where this lives in the directory tree Lives in the Component directory as a subtree split.
  2. Determine what to name this scaffold drupal/core-composer-scaffold
  3. Determine what features it needs to support, and how that differs from the existing plugin. (existing plugin is independent of core, and has logic for different core versions, this one will share core versioning tags etc) See current documentation page: https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...
CommentFileSizeAuthor
#225 2982684-225.patch219.67 KBgreg.1.anderson
#225 2982684-210-to-225-interdiff.txt3.15 KBgreg.1.anderson
#214 2982684-214.patch219.52 KBgreg.1.anderson
#214 2982684-210-to-214-interdiff.txt1.83 KBgreg.1.anderson
#210 2982684-3-alt-209.patch219.66 KBalexpott
#210 206-209-interdiff.txt8.61 KBalexpott
#207 2982684-3-alt-206.patch219.86 KBalexpott
#207 203-206-interdiff.txt24.69 KBalexpott
#203 interdiff.txt1.53 KBMile23
#203 2982684_203.patch226.55 KBMile23
#196 2982684-196.patch226.52 KBgreg.1.anderson
#196 193-to-196-interdiff.txt2.22 KBgreg.1.anderson
#193 2982684-193.patch226.51 KBgreg.1.anderson
#191 2982684-191.patch8.55 KBgreg.1.anderson
#191 189-to-191-interdiff.txt8.55 KBgreg.1.anderson
#189 188-to-189-interdiff.txt875 bytesMixologic
#189 2982684-189.patch225.72 KBMixologic
#188 159-to-188-interdiff.txt85.91 KBgreg.1.anderson
#188 187-to-188-interdiff.txt8.62 KBgreg.1.anderson
#188 2982684-188.patch225.72 KBgreg.1.anderson
#187 2982684-187.patch225.68 KBgreg.1.anderson
#187 159-to-187-interdiff.txt84.82 KBgreg.1.anderson
#187 183-to-187-interdiff.txt24.83 KBgreg.1.anderson
#185 interdiff-2982684-183-185.txt6.23 KByogeshmpawar
#185 2982684-185.patch223.6 KByogeshmpawar
#183 2982684-183.patch223.97 KBgreg.1.anderson
#183 180-to-183-interdiff.txt19.92 KBgreg.1.anderson
#180 2982684-180.patch222.88 KBgreg.1.anderson
#180 159-to-180-interdiff.txt42.59 KBgreg.1.anderson
#159 2982684-159.patch218.59 KBgreg.1.anderson
#159 158-to-159-interdiff.txt3.64 KBgreg.1.anderson
#158 155-to-158-interdiff.txt4.49 KBgreg.1.anderson
#158 2982684-158.patch221.64 KBgreg.1.anderson
#155 2982684-154.patch221.66 KBgreg.1.anderson
#155 149-to-154-interdiff.txt8.34 KBgreg.1.anderson
#153 2982684-153.patch219.34 KBgreg.1.anderson
#153 149-to-153-interdiff.txt5.78 KBgreg.1.anderson
#149 2982684-149.patch218.43 KBMixologic
#149 146-149-interdiff.txt17.17 KBMixologic
#146 145-146-interdiff.txt3.42 KBgreg.1.anderson
#146 2982684-146.patch235.56 KBgreg.1.anderson
#145 144-to-145-interdiff.txt18.51 KBgreg.1.anderson
#145 2982684-145.patch235.46 KBgreg.1.anderson
#144 142-to-144.txt9.86 KBgreg.1.anderson
#144 2982684-144.patch237.16 KBgreg.1.anderson
#142 2982684-2-142.patch230.41 KBalexpott
#142 136-142-interdiff.txt70.17 KBalexpott
#136 2982684-2-136.patch226.76 KBalexpott
#136 131-136-interdiff.txt106.75 KBalexpott
#131 interdiff-130-to-131.txt3.25 KBgreg.1.anderson
#131 2982684-131.patch310.13 KBgreg.1.anderson
#130 interdiff-126-to-130.txt12.46 KBgreg.1.anderson
#130 2982684-130.patch309.51 KBgreg.1.anderson
#127 121_126_interdiff.txt122.15 KBMile23
#126 2982684-125.patch308.7 KBgreg.1.anderson
#124 2982684-124.patch212.35 KBgreg.1.anderson
#123 interdiff-121-to-123.txt23.27 KBgreg.1.anderson
#123 2982684-123.patch209.82 KBgreg.1.anderson
#121 interdiff-118-to-121.txt12.83 KBgreg.1.anderson
#121 2982684-121.patch212.61 KBgreg.1.anderson
#118 2982684-118.patch214.43 KBalexpott
#118 115-118-interdiff.txt84.81 KBalexpott
#115 scaffold_2982684_115.patch213.92 KBgreg.1.anderson
#112 scaffold_2982684_101-to-112_interdiff.txt24.99 KBgreg.1.anderson
#112 scaffold_2982684_112.patch213.64 KBgreg.1.anderson
#108 scaffold_2982684_107.patch315.18 KBgreg.1.anderson
#101 interdiff.txt1.37 KBMile23
#101 2982684_101.patch226.33 KBMile23
#96 2982684_96.patch226.06 KBMile23
#96 interdiff.txt3.19 KBMile23
#93 2982684_93.patch225.97 KBMile23
#89 interdiff.txt113.32 KBMile23
#89 2982684_89.patch113.56 KBMile23
#87 2982684_87.patch90.4 KBMile23
#84 2982684_84.patch90.4 KBMile23
#78 2982684-78.patch92.63 KBvijaycs85
#72 interdiff_70-72.txt662 bytespingwin4eg
#72 2982684-72.patch92.46 KBpingwin4eg
#70 interdiff.txt6.94 KBMile23
#70 2982684_70.patch92.46 KBMile23
#67 interdiff.txt1.66 KBMile23
#67 2982684_67.patch93.48 KBMile23
#66 interdiff.txt3.2 KBMile23
#66 2982684_66.patch95.14 KBMile23
#63 interdiff.txt3.14 KBMile23
#63 2982684_63.patch94.96 KBMile23
#61 interdiff.txt24.56 KBMile23
#61 2982684_61.patch94.22 KBMile23
#53 2982684-53.patch74.37 KBMixologic
#53 interdiff_44_53.txt37.84 KBMixologic
#44 interdiff_35_44.txt31.22 KBMixologic
#44 2982684_44.patch48.18 KBMixologic
#35 2982684-35.patch35.66 KBMixologic
#32 2982684-33.patch35.66 KBMixologic
#31 2982684-31.patch35.65 KBMixologic
#29 2982684-28.patch35.56 KBMixologic
#27 2982684-26.patch35.6 KBMixologic
#25 2982684-24.patch35.48 KBMixologic
#23 2982684-23.patch1.83 KBMixologic
#20 2982684-19.patch1.83 KBMixologic
#15 2982684-15.patch35.46 KBwebflo
#13 2982684-13.interdiff.txt1.92 KBwebflo
#13 2982684-13.patch35.46 KBwebflo
#11 2982684-10.patch35.76 KBwebflo
#11 2982684.10.interdiff.txt752 byteswebflo
#9 2982684-9.interdiff.txt1.1 KBwebflo
#9 2982684-9.patch35.83 KBwebflo
#7 2982684-7.patch35.96 KBwebflo
#4 2982684-4.patch36.1 KBwebflo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mixologic created an issue. See original summary.

webchick’s picture

Priority: Normal » Major

This is a core requirement of the Composer initiative, so escalating to Major.

Mixologic’s picture

This exists now:
https://github.com/drupal/drupal-scaffold

The main difference with it and the original its forked from is that every file, including all the README.txts are being synced over.

webflo’s picture

Status: Active » Needs review
FileSize
36.1 KB

This is mostly the plugin the same code as the current stable release of the plugin. I removed the support for multiple Drupal versions in the plugin, because we release the plugin together with Drupal core anyway.

Lets see if the tests run on Drupal CI.

Status: Needs review » Needs work

The last submitted patch, 4: 2982684-4.patch, failed testing. View results

webflo’s picture

Status: Needs work » Needs review
  1. +++ b/core/scripts/composer/scaffold/composer.json
    @@ -0,0 +1,24 @@
    +            "DrupalComposer\\DrupalScaffold\\": "src/"
    

    The namespace should be updated.

  2. +++ b/core/scripts/composer/scaffold/composer.json
    @@ -0,0 +1,24 @@
    +    "autoload-dev": {
    +        "classmap": [
    +            "../../../lib/Drupal/Core/Composer/Composer.php"
    +        ]
    +    },
    

    Not required anymore. Should be removed.

  3. +++ b/core/scripts/composer/scaffold/phpunit.xml.dist
    @@ -0,0 +1,19 @@
    +    <php>
    +        <env name="COMPOSER_ROOT_VERSION" value="999.0.999" />
    +    </php>
    

    Not required anymore.

webflo’s picture

FileSize
35.96 KB

Reroll, my repo was outdated.

Status: Needs review » Needs work

The last submitted patch, 7: 2982684-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

webflo’s picture

Status: Needs work » Needs review
FileSize
35.83 KB
1.1 KB

Status: Needs review » Needs work

The last submitted patch, 9: 2982684-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

webflo’s picture

Status: Needs review » Needs work

The last submitted patch, 11: 2982684-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

webflo’s picture

Adding "prefer-stable: true" for faster tests, because otherwise all packages are fetched from git repos because of minimum-stability: dev.

Status: Needs review » Needs work

The last submitted patch, 13: 2982684-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

webflo’s picture

Status: Needs work » Needs review
FileSize
35.46 KB

Just the CS fix. 👌

Status: Needs review » Needs work

The last submitted patch, 15: 2982684-15.patch, failed testing. View results

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mixologic’s picture

+++ b/core/drupalci.yml
@@ -20,36 +20,8 @@ build:
+          - "cd ${SOURCE_DIR}/core/scripts/composer/scaffold && sudo -u www-data ./vendor/bin/phpunit"

Need to tell phpunit to put out junit in a folder where drupalci can find it. Im looking now where that should be.

bojanz’s picture

1) The plugin has optional integration with hirak/prestissimo. Feels a bit odd to not have a decision here, why not require it if it's nice to have?

2) The plugin doesn't follow Drupal coding standards. CamelCase is used for method arguments sometimes, and phpdoc is generally missing.

Example:

"+ /**
+ * @var \Composer\Util\RemoteFilesystem
+ */
+ protected $remoteFilesystem;
+
+ /**
+ * @var \Composer\IO\IOInterface
+ */
+ protected $io;
+
+ /**
+ * @var bool
+ *
+ * A boolean indicating if progress should be displayed.
+ */
+ protected $progress;
+
+ protected $source;
+ protected $filenames;
+ protected $fs;

3) The plugin specifies PHP 5.4.5 as the minimum. Why wouldn't this minimum be in sync with core?

4)

+ public function fetch($version, $destination, $override) {

$override is a boolean, why doesn't it have a default?

5) README bikeshedding:

+It is recommended that the vendor directory be placed in its standard location
+at the project root, outside of the Drupal root; however, the location of the
+vendor directory and the name of the Drupal root may be placed in whatever
+location suits the project.

I'd switch the two sentences around. Say that both vendor locations (in docroot/outside docroot) are supported, but that outside docroot is recommended.

+The `initial` hash lists files that should be copied over only if they do not
+exist in the destination. The key specifies the path to the source file, and
+the value indicates the path to the destination file.

It was not obvious to me based on name alone what the "initial" key does. Might make sense to have a longer but more descriptive name.

+When using Composer to install or update the Drupal development branch, the
+scaffold files are always taken from the HEAD of the branch (or, more
+specifically, from the most recent development .tar.gz archive). This might
+not be what you want when using an old development version (e.g. when the
+version is fixed via composer.lock). To avoid problems, always commit your
+scaffold files to the repository any time that composer.lock is committed.
+Note that the correct scaffold files are retrieved when using a tagged release
+of `drupal/core` (recommended).

Wasn't obvious to me that this warning applies only when using a -dev version of core. Would make sense to preface the explanation with that.

Mixologic’s picture

adding --log-junit /var/lib/drupalci/workdir/container_command.drupal_scaffold/junitxml/drupal_scaffold.xml
to see if we can get drupalci to view the test result.

Mixologic’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: 2982684-19.patch, failed testing. View results

Mixologic’s picture

Status: Needs work » Needs review
FileSize
1.83 KB

Wrong patch. Here's the right one

Status: Needs review » Needs work

The last submitted patch, 23: 2982684-23.patch, failed testing. View results

Mixologic’s picture

Status: Needs work » Needs review
FileSize
35.48 KB

Okay, here again.

Status: Needs review » Needs work

The last submitted patch, 25: 2982684-24.patch, failed testing. View results

Mixologic’s picture

Status: Needs work » Needs review
FileSize
35.6 KB

Lets mkdir the artifact dir and put the files there.

Status: Needs review » Needs work

The last submitted patch, 27: 2982684-26.patch, failed testing. View results

Mixologic’s picture

Status: Needs work » Needs review
FileSize
35.56 KB

remove the sudo's

Status: Needs review » Needs work

The last submitted patch, 29: 2982684-28.patch, failed testing. View results

Mixologic’s picture

Status: Needs work » Needs review
FileSize
35.65 KB

Added sudos back in, hacked in a chmod.

Mixologic’s picture

whoops

The last submitted patch, 31: 2982684-31.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 32: 2982684-33.patch, failed testing. View results

Mixologic’s picture

more tweakage

Mixologic’s picture

Status: Needs work » Needs review
kim.pepper’s picture

Excited to see this progressing!

My feedback is just basically codesniffer issues. Should we add core/scripts/composer/scaffold/src/ to the phpcs.xml and do a phpcbf to automatically fix them?

  1. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    + * Downloads all required files and writes it to the file system.
    

    s/it/them

  2. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  protected $source;
    +  protected $filenames;
    +  protected $fs;
    

    Need docblocks for these?

  3. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +   * Downloads all required files and writes it to the file system.
    

    s/it/them

  4. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  public function setFilenames(array $filenames) {
    

    Missing @param docblock

  5. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  protected function getUri($filename, $version) {
    

    Missing @param docblocks

  6. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    + * Core class of the plugin, contains all logic which files should be fetched.
    

    Needs proper grammar.

  7. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  const PRE_DRUPAL_SCAFFOLD_CMD = 'pre-drupal-scaffold-cmd';
    +  const POST_DRUPAL_SCAFFOLD_CMD = 'post-drupal-scaffold-cmd';
    

    Needs docblocks

  8. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +   * @param \Composer\Composer $composer
    +   * @param \Composer\IO\IOInterface $io
    

    Needs comments

  9. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  protected function manualLoad() {
    

    Needs docblock

  10. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  protected function getCorePackage($operation) {
    

    Needs description

  11. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +   * @return array
    ...
    +   * @return array
    ...
    +   * @return array
    ...
    +    return $result;
    ...
    +   * @return array
    

    string[] ?

  12. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  protected function getExcludesDefault() {
    

    Needs @return

  13. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  protected function getIncludesDefault() {
    

    Needs @return

  14. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  protected function getInitialDefault() {
    

    Needs @return

phenaproxima’s picture

Status: Needs review » Needs work

Nice to see this is working, and that Drupal CI is passing its tests.

This is a pretty shallow review; I didn't get all the way through it. There are a lot of coding standards things we need to fix first, before this is really "reviewable", because many methods are undocumented and without that, it's going to be hard to parse what's going on.

  1. +++ b/core/scripts/composer/scaffold/.gitignore
    @@ -0,0 +1,2 @@
    +/vendor
    +/composer.lock
    

    Do we still need this?

  2. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +`index.php`, `update.php`, …) when using `drupal/core` via Composer.
    

    Can we replace the ellipsis with "etc"? And can we say "requiring" instead of "using", since the key in composer.json is "require"?

  3. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +vendor directory and the name of the Drupal root may be placed in whatever
    

    What does "name of the Drupal root" mean?

  4. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +Run `composer require drupal/drupal-scaffold` in your composer
    

    I think that Composer should be consistently capitalized. Shouldn't it be capitalized here?

  5. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +You can configure the plugin with providing some settings in the `extra` section
    

    "...by providing some settings..."

  6. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +scaffold files from; the default source is drupal.org. The literal string
    

    The default source is cgit.drupalcode.org, not drupal.org.

  7. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +`{version}` in the `source` option is replaced with the current version of
    

    It seems to me that the {path} and {version} placeholders assume a cgit.drupalcode.org-style URL. Perhaps this should be abstracted a bit (maybe specify a class which can be used to resolve the URL of each scaffold file)? If not, maybe we should hard-code cgit.drupalcode.org for now and open a follow-up to reintroduce this option.

  8. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +When setting `omit-defaults` to `true`, neither the default excludes nor the
    +default includes will be provided; in this instance, only those files explicitly
    +listed in the `excludes` and `includes` options will be considered. If
    +`omit-defaults` is `false` (the default), then any items listed in `excludes`
    +or `includes` will be in addition to the usual defaults.
    +
    +The `initial` hash lists files that should be copied over only if they do not
    +exist in the destination. The key specifies the path to the source file, and
    +the value indicates the path to the destination file.
    

    I find the combination of 'includes', 'excludes', 'omit-defaults', and 'initial' to be rather confusing. I propose that we remove these options entirely in favor of a single option, called 'files', which explicitly explains what do with each scaffold file -- i.e., where to save it, and when/if to overwrite. It would look something like this:

    "extra": {
      "drupal-scaffold": {
        "files" {
          ".csslintrc": {
            "destination-path": ".csslintrc.dist",
            "overwrite": "never"
          },
          "sites/default/default.settings.php": {
            "overwrite": "always"
          }
        }
      }
    }
    

    Obviously, this would break BC with the current plugin. So whether we can even do this really depends on whether we will be taking ownership of the drupal-composer/drupal-scaffold namespace on Packagist. If we are, then this will mean that we're rolling a new major version of the plugin. If we're not, then we can do this with no consequences to existing sites that use drupal-composer/drupal-scaffold.

  9. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +or "@composer drupal:scaffold" in Composer Scripts.
    

    This should be "composer.json scripts:"

  10. +++ b/core/scripts/composer/scaffold/README.md
    @@ -0,0 +1,127 @@
    +above). After running `composer install` for the first time commit the scaffold
    +files to your repository.
    

    This last sentence needs to be rephrased: "Commit the scaffold files to your repository after running `composer install` for the first time."

  11. +++ b/core/scripts/composer/scaffold/composer.json
    @@ -0,0 +1,22 @@
    +    "name": "drupal/drupal-scaffold",
    

    Apropos to my settings proposal before -- this would be a new namespace (the current plugin is drupal-composer/drupal-scaffold).

  12. +++ b/core/scripts/composer/scaffold/composer.json
    @@ -0,0 +1,22 @@
    +    "description": "Composer Plugin for updating the Drupal scaffold files when using drupal/core",
    

    "Plugin" should be lowercase.

  13. +++ b/core/scripts/composer/scaffold/src/CommandProvider.php
    @@ -0,0 +1,21 @@
    +namespace DrupalComposer\DrupalScaffold;
    

    Seems to me that this should be moved into the Drupal\Composer namespace, since this is being rolled into Drupal itself, and also because we might eventually add more commands to this command provider.

  14. +++ b/core/scripts/composer/scaffold/src/CommandProvider.php
    @@ -0,0 +1,21 @@
    +/**
    + * List of all commands provided by this package.
    + */
    +class CommandProvider implements CommandProviderCapability {
    

    Doc comment needs to start with a verb.

  15. +++ b/core/scripts/composer/scaffold/src/DrupalScaffoldCommand.php
    @@ -0,0 +1,36 @@
    +class DrupalScaffoldCommand extends BaseCommand {
    

    Can we just call this "ScaffoldCommand"? It's already very freaking obvious that it has to do with Drupal. :)

  16. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  /**
    +   * @var \Composer\Util\RemoteFilesystem
    +   */
    +  protected $remoteFilesystem;
    +
    +  /**
    +   * @var \Composer\IO\IOInterface
    +   */
    +  protected $io;
    

    These need descriptions.

  17. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  /**
    +   * @var bool
    +   *
    +   * A boolean indicating if progress should be displayed.
    +   */
    +  protected $progress;
    

    Description needs to come before @var.

  18. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  protected $source;
    +  protected $filenames;
    +  protected $fs;
    

    These all need doc comments.

  19. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  /**
    +   * Constructs this FileFetcher object.
    +   */
    +  public function __construct(RemoteFilesystem $remoteFilesystem, $source, IOInterface $io, $progress = TRUE) {
    

    The parameters need to be documented.

  20. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +    $this->fs = new Filesystem();
    

    This should be optionally injectable.

  21. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  /**
    +   * Downloads all required files and writes it to the file system.
    +   */
    +  public function fetch($version, $destination, $override) {
    

    The parameters need to be documented.

  22. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  /**
    +   * Set filenames.
    +   */
    +  public function setFilenames(array $filenames) {
    

    Ditto.

  23. +++ b/core/scripts/composer/scaffold/src/FileFetcher.php
    @@ -0,0 +1,87 @@
    +  /**
    +   * Replace filename and version in the source pattern with their values.
    +   */
    +  protected function getUri($filename, $version) {
    

    Ditto.

  24. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  const PRE_DRUPAL_SCAFFOLD_CMD = 'pre-drupal-scaffold-cmd';
    +  const POST_DRUPAL_SCAFFOLD_CMD = 'post-drupal-scaffold-cmd';
    

    These constants need doc comments.

  25. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  /**
    +   * @var \Composer\Composer
    +   */
    +  protected $composer;
    +
    +  /**
    +   * @var \Composer\IO\IOInterface
    +   */
    +  protected $io;
    

    Need descriptions.

  26. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  /**
    +   * @var bool
    +   *
    +   * A boolean indicating if progress should be displayed.
    +   */
    +  protected $progress;
    

    Description before @var.

  27. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +   * @param \Composer\Composer $composer
    +   * @param \Composer\IO\IOInterface $io
    +   */
    +  public function __construct(Composer $composer, IOInterface $io) {
    

    Parameters need descriptions.

  28. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +    // Pre-load all of our sources so that we do not run up
    +    // against problems in `composer update` operations.
    +    $this->manualLoad();
    

    This comment could use more information.

  29. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  protected function manualLoad() {
    

    Needs doc comment.

  30. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +  /**
    +   * @param $operation
    +   * @return mixed
    +   */
    +  protected function getCorePackage($operation) {
    

    Doc comment needs description, parameter descriptions, and @return.

  31. +++ b/core/scripts/composer/scaffold/src/Handler.php
    @@ -0,0 +1,398 @@
    +    $relativeVendorPath = rtrim($relativeVendorPath, '/');
    

    All variable names should be $snake_case.

webflo’s picture

Thanks @kim.pepper and @phenaproxima for the reviews. I will fix all the things in the original drupal-scaffold plugin and post a new patch afterwards. I would like to keep both code bases every close together, at least as long the behavior does not change.

AaronMcHale’s picture

Love the progress here, keep it up! Just doing a bit of cleaning.

Mixologic’s picture

I was experimenting with having a lock file in the template, and it turns out that the scaffold plugin does not trigger if theres a lock file.

I had to add the following to the plugin to make it work with the right event:

diff --git a/src/Plugin.php b/src/Plugin.php
index 76677cb..e48f0be 100644
--- a/src/Plugin.php
+++ b/src/Plugin.php
@@ -52,6 +52,7 @@ class Plugin implements PluginInterface, EventSubscriberInterface, Capable {
       PackageEvents::POST_PACKAGE_INSTALL => 'postPackage',
       PackageEvents::POST_PACKAGE_UPDATE => 'postPackage',
       ScriptEvents::POST_UPDATE_CMD => 'postCmd',
+      ScriptEvents::POST_CREATE_PROJECT_CMD => 'postCmd',
       PluginEvents::COMMAND => 'cmdBegins',
     ];
   }
Mile23’s picture

  1. +++ b/core/scripts/composer/scaffold/tests/FetcherTest.php
    @@ -0,0 +1,79 @@
    +class FetcherTest extends TestCase {
    
    +++ b/core/scripts/composer/scaffold/tests/PluginTest.php
    @@ -0,0 +1,148 @@
    +/**
    + * Tests composer plugin functionality.
    + */
    +class PluginTest extends TestCase {
    

    Needs docblock and importantly @group. Also needs a core test suite or at least some form of test discovery.

    This is similar to the problems in #2943856: [meta] Reorganize Components so they are testable in isolation but here we have the opportunity to set up the tests the way they should be for a subtree split project. This might be better as a follow-up but maybe not.

  2. +++ b/core/scripts/composer/scaffold/composer.json
    --- /dev/null
    +++ b/core/scripts/composer/scaffold/phpunit.xml.dist
    

    Really not sure about the phpunit.xml.dist, because of the way we run tests for core. Do we add a test runner phase where we run these tests independently? Do we discover them and run them in one big run-tests?

Mixologic’s picture

Okey dokes. I did a bunch of stuff on this to keep momentum moving forward.

I've implemented most of the reviews in #37/#38, but there are still some bits that are less "review" and more "architecture decisiony" etc.

#37 1-14 : all done. I did not make any changes to phpcs.xml for now, I just ran them locally.
#38-1: depends on how we test the components/plugins. See last half of #2 of #43.
#38-2: done
#38-3: reworded to be clearer, I hope.
#38-4-6: fixed.
#38-7: {path} and {version} are presumably the two things you would need from any http api to a git repo. Seems like cgit vs gitlab vs github those would all work.
#38-8: I agree that we ought to have a better interface to make it clear the difference between "copy this file once, and never change it, to copy this file always if it changes, vs default omitting etc. I didnt address this because too much other tidying went into this patch, and it seemed like we ought to chat through this.
#38-9: I dont think this should be 'composer.json' scripts. It refers to using it *within* a composer script you write yourself.
#38-10: done
#38-11: yes. This will be an official subtree split into drupal/drupal-scaffold, and shouldnt have much conflicts with upgrades from the old drupal-project/drupal-scaffold. This entire project is for getting new things off the ground, but we should discuss the upgrade path from existing sites to this mechanism.
#38-12: Done
#38-13: I didnt change the namespace, but yes, it should be changed.
#38-14: done
#38-15: no, this is defining a new *composer* command and as such has the potential to namespace collide with somebody elses composer commands that are being defined external packages. So, Drupal ensures we're not going to have collisions.
#38-16-19: done
#38-20: Agreed, injectable, but I didnt do it yet.
#38-21-31: all donezo.

#39 - changes are all pushed upstream do drupal/drupal-scaffold, so maybe we can make a pull request somehow.
#43: I didnt change anything in the testing, so yes that should still be addressed.

This patch cleans up the aforementioned review points, and also introduces an additonal thing: It implements the code cleanup facility from core that cleans the vendor packages. This way, since the scaffold will be the first thing downloaded, it will be able to be called from the template, even on the very first installation from a lock file in a composer project.

There is a travis test over at https://travis-ci.org/drupal/drupal-project-legacy/builds/448029534 showing that this, in conjunction with the lock file idea from #42 means that a template, and a scaffold plugin are all we need.

So one question is how do we make sure this scaffold plugin will satisfy the build needs of a distribution maintainer?

Other questions are the bold points above that we should still address. 1. the api and its design, and whether it needs to be BC with existing scaffold, or whether thats on its own and we need to provide transition instructions.

2. What should the namespace be, if this is external to Drupal/Composer and will live outside of core on its own?

3. What should we do about testing external plugins in general. Maybe now is, or is not, the time to make them testable on their own?

Mixologic’s picture

Status: Needs work » Needs review

->NR

fgm’s picture

Issue summary: View changes
Mile23’s picture

#44.1: If we were going to do a PR for this new work, then it makes sense to say it needs BC. But it's really a fork. I'm not sure there's much difference in behavior, but we should generally try to document what's different. We could even name it drupal/scaffold.

#44.2: How about PSR-4: DrupalScaffold => src? I think it should reside outside of core/, since core is where drupal/core lives, but that's another bikeshed for another day.

#44.3: The changes to drupalci.yml in #44 bring to mind: #2961531: Allow host_command and container_command to create artifacts and #2837112: Add a test definition for a generic PHPUnit test run The big question is whether we'd want that on every test run of core, or whether it should have some other expectation. The test run takes less than a minute locally, so maybe not a hardship.

alexpott’s picture

+++ b/core/scripts/composer/scaffold/src/Handler.php
@@ -0,0 +1,649 @@
+  protected static $packageToCleanup = [
+    'behat/mink' => ['tests', 'driver-testsuite'],
+    'behat/mink-browserkit-driver' => ['tests'],
+    'behat/mink-goutte-driver' => ['tests'],
+    'drupal/coder' => ['coder_sniffer/Drupal/Test', 'coder_sniffer/DrupalPractice/Test'],
+    'doctrine/cache' => ['tests'],
+    'doctrine/collections' => ['tests'],
+    'doctrine/common' => ['tests'],
+    'doctrine/inflector' => ['tests'],
+    'doctrine/instantiator' => ['tests'],
+    'egulias/email-validator' => ['documentation', 'tests'],
+    'fabpot/goutte' => ['Goutte/Tests'],
+    'guzzlehttp/promises' => ['tests'],
+    'guzzlehttp/psr7' => ['tests'],
+    'jcalderonzumba/gastonjs' => ['docs', 'examples', 'tests'],
+    'jcalderonzumba/mink-phantomjs-driver' => ['tests'],
+    'masterminds/html5' => ['test'],
+    'mikey179/vfsStream' => ['src/test'],
+    'paragonie/random_compat' => ['tests'],
+    'phpdocumentor/reflection-docblock' => ['tests'],
+    'phpunit/php-code-coverage' => ['tests'],
+    'phpunit/php-timer' => ['tests'],
+    'phpunit/php-token-stream' => ['tests'],
+    'phpunit/phpunit' => ['tests'],
+    'phpunit/php-mock-objects' => ['tests'],
+    'sebastian/comparator' => ['tests'],
+    'sebastian/diff' => ['tests'],
+    'sebastian/environment' => ['tests'],
+    'sebastian/exporter' => ['tests'],
+    'sebastian/global-state' => ['tests'],
+    'sebastian/recursion-context' => ['tests'],
+    'stack/builder' => ['tests'],
+    'symfony/browser-kit' => ['Tests'],
+    'symfony/class-loader' => ['Tests'],
+    'symfony/console' => ['Tests'],
+    'symfony/css-selector' => ['Tests'],
+    'symfony/debug' => ['Tests'],
+    'symfony/dependency-injection' => ['Tests'],
+    'symfony/dom-crawler' => ['Tests'],
+    // @see \Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest
+    // 'symfony/event-dispatcher' => ['Tests'],
+    'symfony/http-foundation' => ['Tests'],
+    'symfony/http-kernel' => ['Tests'],
+    'symfony/process' => ['Tests'],
+    'symfony/psr-http-message-bridge' => ['Tests'],
+    'symfony/routing' => ['Tests'],
+    'symfony/serializer' => ['Tests'],
+    'symfony/translation' => ['Tests'],
+    'symfony/validator' => ['Tests', 'Resources'],
+    'symfony/yaml' => ['Tests'],
+    'symfony-cmf/routing' => ['Test', 'Tests'],
+    'twig/twig' => ['doc', 'ext', 'test'],
+  ];

It'd be a shame to have to maintain this list twice. Here and in \Drupal\Core\Composer\Composer::$packageToCleanup

Can we make this a public static and only have it once?

Also having both core/scripts/composer and core/lib/Drupal/Core/Composer is a bit confusing.

Mile23’s picture

Mile23’s picture

Mile23’s picture

Poking at #2837112-6: Add a test definition for a generic PHPUnit test run to support an arbitrary phpunit test run.

Mixologic’s picture

That the hacks to drupalci.yml are merely a proof that the tests we have pass, but are not intended in any way to be part of the eventual patch.

I do not think that testing components in isolation is in scope at all, so I'm working on moving the tests where everything else lives at this point (core/tests/Drupal/Tests/Component).

Mixologic’s picture

Okay, so I took in the feedback from #48, and realized that we don't actually need any of the vendor cleanup scripts in Drupal\Core\Composer\Composer.php, because those exist as a security safety mechanism, and one of the fundamental goals of the composer initiative is that *nobody should be running the git repo in production*. As such, we only need that mechanism to exist when you have the project created with composer originally (via packaging or via composer). So rather than have that in two places, I've removed it entirely.

Also, I moved the scaffold to a component, moved the tests to where they would live, and in this patch, adjusted the drupalci.yml to only run the unit tests (to see if the 2 existing tests pass without attempting to redo testing for components.

What remains to be done here is coming up with a better design for handling existing files. Im wondering whether we need to concern ourselves with existing users of the drupal-project/drupal-scaffold, or whether that can come later, like a script to help people transition from one to the other.

Mixologic’s picture

Also, I left in a 'deprecation notice' for the vendortestcleanup script in Drupal\Core\Composer\Composer.php This is because people have taken to meddling with their drupal/drupal composer.json, swapping the 'replace' with 'require' for drupal/core, and as such would end up with calls to these scripts even after an upgrade, and if that script call completely went away, composer update would crash for those users.

Status: Needs review » Needs work

The last submitted patch, 53: 2982684-53.patch, failed testing. View results

Mile23’s picture

  1. +++ b/composer.json
    @@ -50,8 +50,6 @@
    -        "post-package-install": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
    -        "post-package-update": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
    
    +++ b/core/lib/Drupal/Core/Composer/Composer.php
    @@ -192,82 +138,17 @@ public static function upgradePHPUnitCheck($phpunit_version) {
    +   * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
    

    Well, hang on a sec, because we're still going to allow people to use Composer to put their vendor directory in the docroot. In fact, that's the first template we're going to make, right? So we still need to do the cleanup.

    Let's do the move-or-remove as a follow-up. Then we have the scaffold in core sooner and we can iterate sooner and so can drupal-composer.

  2. Test result:
    1) Drupal\Tests\Component\Scaffold\FetcherTest::testFetch
    Error: Class 'Composer\Util\Filesystem' not found
    

    Add composer/composer as a dev dependency. Then we're back to exactly why we should test these in isolation, and why merge is bad. #2943856: [meta] Reorganize Components so they are testable in isolation

fgm’s picture

@mile23 about 1. are we, really ? is there any remaining reason to promote that template versus the docroot one level down ?

Mile23’s picture

Mixologic’s picture

#1 We still do the cleanup. Its been moved into the scaffold component, and is a plugin now. We dont need to do the cleanup when somebody runs composer install on a git clone of the drupal repo, so we no longer need the scripts in Composer.php. And we need it in the scaffold in order to "allow people to use Composer to put their vendor directory in the docroot", thus it cant be a followup.

#2 While it would be nice to test these in isolation, and nice to get rid of the merge, thats not the situation we're dealing with currently, and can be postponed until after we get this composer work done. Testing the components in isolation has a whole slew of implications, and is a major undertaking itself.

In trying to get the tests to pass locally, we might need to figure out better testing. Right now the scaffold tests assume that it is being tested in isolation, so we'll probably need to redo the way we test these.

fgm’s picture

@mile23: thanks I can't believe I missed that issue.

Mile23’s picture

Status: Needs work » Needs review
FileSize
94.22 KB
24.56 KB

Fixed the assumptions baked into PluginTest, so now it knows about the rest of the core codebase.

It still fails locally, but for real reasons. I have to stop here for the day or I'd investigate further.

Also added composer/composer as a dev requirement.

Re: #59.1: I don't see where the removal of test files is performed. Never mind.

Status: Needs review » Needs work

The last submitted patch, 61: 2982684_61.patch, failed testing. View results

Mile23’s picture

Status: Needs work » Needs review
FileSize
94.96 KB
3.14 KB

PluginTest now runs Composer in a separate process, and the Scaffold component now has a classmap instead of PSR-4.

That's how I got the tests to pass locally.

We'd much prefer to have PSR-4 since a classmap for our plugin will eat up memory on every request.

Status: Needs review » Needs work

The last submitted patch, 63: 2982684_63.patch, failed testing. View results

Mile23’s picture

Exit Code: 1(General error)

Working directory: /tmp/drupal-scaffold

Output:
================


Error Output:
================
sh: 0: getcwd() failed: No such file or directory
Composer could not find a composer.json file in 
To initialize a project, please create a composer.json file as described in the https://getcomposer.org/ "Getting Started" section

I think we need to find a tmp dir within DRUPAL_ROOT/sites/simpletest if we're running under the testbot. This means adding that case as a conditional, since the test should also be able to run without the rest of the core codebase. (It's a component, after all.)

+++ b/core/lib/Drupal/Component/Scaffold/PrestissimoFileFetcher.php
@@ -0,0 +1,100 @@
+ * Extends the default FileFetcher for parallel downloads.
+ *
+ * If hirak/prestissimo is available, it will use, otherwise it will default to
+ * FileFetcher default behavior.

Scaffold's composer.json should suggest hirak/prestissimo.

Mile23’s picture

Status: Needs work » Needs review
FileSize
95.14 KB
3.2 KB

The test tries to use the simpletest directory.

Mile23’s picture

Restores the full core/drupalci.yml.

Mile23’s picture

+++ b/core/lib/Drupal/Component/Scaffold/Handler.php
@@ -0,0 +1,649 @@
+  /**
+   * Pre-load all of our sources on initial load.
+   *
+   * This ensures that we will not get a more recent version of one of
+   * our classes e.g. after a 'composer update' operation.
+   */
+  protected function manualLoad() {
+    $src_dir = __DIR__;
+
+    $classes = [
+      'CommandProvider',
+      'DrupalScaffoldCommand',
+      'FileFetcher',
+      'PrestissimoFileFetcher',
+    ];
+
+    foreach ($classes as $src) {
+      if (!class_exists('\\Drupal\\Component\\Scaffold\\' . $src)) {
+        include "{$src_dir}/{$src}.php";
+      }
+    }
+  }

+++ b/core/lib/Drupal/Component/Scaffold/composer.json
@@ -0,0 +1,30 @@
+    "autoload": {
+        "classmap": [
+            "CommandProvider.php",
+            "DrupalScaffoldCommand.php",
+            "FileFetcher.php",
+            "Handler.php",
+            "Plugin.php",
+            "PrestissimoFileFetcher.php"
+        ]
+    },

Somewhere from #63 we're able to get the plugin classes to load, but it's also quite strange that we need to use a classmap instead of PSR-4. This is the main not-ready-for-prime-time aspect of this. This is only to get it to run under test. It works fine as PSR-4 in other circumstances.

larowlan’s picture

+++ b/core/lib/Drupal/Component/Scaffold/Handler.php
@@ -0,0 +1,649 @@
+return require __DIR__ . '/$relativeVendorPath/autoload.php';

i might be missing something here but shouldn't this be double quotes so that $relativeVendorPath is replaced?

Mile23’s picture

Good sleuthing but that's inside a heredoc, so the variable should expand.

It turns out I never did try reverting to PSR-4 after isolating the Composer calls within their own process. So here's a patch with that, and it works locally. Will the testbot disagree?

Also CS from #67.

Status: Needs review » Needs work

The last submitted patch, 70: 2982684_70.patch, failed testing. View results

pingwin4eg’s picture

Status: Needs work » Needs review
FileSize
92.46 KB
662 bytes

@Mile23 you've changed PrestissimoFileFetcher constructor arguments order, but not in the caller. Here's the fix.

Mile23’s picture

Yah you get to the end of a thing and you make that kind of mistake, thanks.

+++ b/core/lib/Drupal/Component/Scaffold/Handler.php
@@ -270,7 +270,7 @@
-    $fetcher = new PrestissimoFileFetcher($remoteFs, $options['source'], $this->io, $this->progress, $this->composer->getConfig());
+    $fetcher = new PrestissimoFileFetcher($remoteFs, $options['source'], $this->io, $this->composer->getConfig(), $this->progress);

+1

Mile23’s picture

Patch still applies, re-running test.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

My concern here is it relies on cgit.drupal.org, see https://github.com/drupal-composer/drupal-scaffold/issues/71 for where this has been an issue.

vijaycs85’s picture

Issue tags: +Needs reroll
vijaycs85’s picture

Mixologic’s picture

re#76: we're about 2 weeks away from cgit being replaced with gitlab, which should be tremendously more stable, and will have all appropriate redirects.

larowlan’s picture

+++ b/composer.lock
@@ -3634,6 +3875,72 @@
+            "name": "justinrainbow/json-schema",

@@ -4541,6 +4848,99 @@
+            "name": "seld/jsonlint",

are these changes intended? bad reroll?

vijaycs85’s picture

@larowlan definitely exist in #72 and #70.

Mile23’s picture

$ composer why justinrainbow/json-schema
composer/composer  1.8.0  requires  justinrainbow/json-schema (^3.0 || ^4.0 || ^5.0)  

Same for seld/jsonlint.

If you say composer why composer/composer, it tells you nothing depends on composer/composer. And that's because we merge the scaffold instead of requiring it.

Here's why we have composer/composer:

+++ b/core/lib/Drupal/Component/Scaffold/composer.json
@@ -0,0 +1,25 @@
+    "require-dev": {
+        "composer/composer": "^1.8",
+        "symfony/process": "~3.4.0"
amateescu’s picture

justinrainbow/json-schema was just committed to 8.8.x (and soon to 8.7.x too) as part of #2843147: Add JSON:API to core as a stable module (see #3036210-33: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses), so this patch needs a reroll :)

Mile23’s picture

Needed re-roll per #83.

Mile23’s picture

Updating IS a little.

The 'needs followup' tag was from #49, and refers to #2998829: Add a composer script method to Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup which we've decided to implement as part of the plugin here.

Adding 'needs change record update' because the change record is a stub.

Mile23’s picture

Updated change record.

Mile23’s picture

Needed re-roll.

Mixologic’s picture

So, documenting some impending changes wants:

@phenaproxima wants to polish the design of the configuration of the scaffold to make the includes/excludes/defaults a more sensible design.

@alexpott would like to see a way for the scaffold to have a fallback for when retrieving files it has somewhere else to pull from.

Potentially instead of retrieving the files from drupal.org git repo we should take anything that people want to scaffold, and move them into a subdirectory of /core
And if we have a plugin that is capable of "moving files around" then maybe it should be able to "move any files around" as part of composition such that we can eventually use it to solve the assets

Mile23’s picture

Based on heroic efforts by @grasmash, @Mixologic, @greg.1.anderson, @phenaproxima and others at DrupalCon 2019 in Seattle, we have new patch based on work being carried out in https://github.com/grasmash/composer-scaffold

This new patch addresses concerns from #88.

This patch is part of a WiP which will likely reflect more 'upstream' changes in the near future. Presented here in order to get an opinion from the testbot, and any early review people would like to offer.

Status: Needs review » Needs work

The last submitted patch, 89: 2982684_89.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Mile23’s picture

Status: Needs work » Needs review

Unrelated fail, restarting test.

greg.1.anderson’s picture

All of the issues in https://github.com/grasmash/composer-scaffold have been resolved. Thanks @mile23 for offering to re-roll the patch. Thanks to everyone who collaborated on this at DrupalCon Seattle and afterwards.

Mile23’s picture

Let's try this out. :-)

I have a fork of gramash/composer-scaffold over here: https://github.com/paul-m/composer-scaffold/tree/component This branch contains all the changes you can do in order to make the repo look like a Drupal component, without actually making it a component. You can see it passing tests here: https://travis-ci.org/paul-m/composer-scaffold/builds/534277774

A couple things to note about the patch:

  • Please read the README and see all the neato things it does. :-)
  • It's assumed this patch will be applied to 8.8.x, which will have a minimum PHP version requirement of 7.0.8: #3053363: Remove support for PHP 5 in Drupal 8.8
  • The scaffold component says require-dev: "phpunit/phpunit": "^4.8.35 || ^6.5" and this is a lie. The tests use PHPUnit 6 idioms, but we can't merge the component unless its constraints mimic those of core. Until core drops PHPUnit 4 support formally we can't be honest about the scaffold.
  • The tests should eventually be ported over to the build test framework from #3031379: Add a new test type to do real update testing. If that issue is committed first, we should do that here. Otherwise, needs a followup.

Significantly different from #89, so no interdiff.

This is the work of @grasmash, @Mixologic, @greg.1.anderson, @phenaproxima, @jhedstrom and probably a bunch of others. :-)

Fingers crossed....

Status: Needs review » Needs work

The last submitted patch, 93: 2982684_93.patch, failed testing. View results

greg.1.anderson’s picture

Three sets of errors in the previous patch. The first and last appear to be unrelated. The middle one is caused by some scaffold tests that use git; apparently, the drupal.org test runner needs a little more setup than TravisCI does.

Mile23’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
226.06 KB

The first fail was DrupalComponentTest, which was only making sure that our license files are identical. (They are now.)

The second was from ManageGitIgnoreTest, where we're making commits to a repo we created in the test. I've addressed it here, but it might be better to get DrupalCI configured with a global git user, because we're probably going to be doing this kind of thing a lot after #3031379: Add a new test type to do real update testing

The third is a big ol' shrug, and I think it's unrelated. Let's see if it fails again here.

Status: Needs review » Needs work

The last submitted patch, 96: 2982684_96.patch, failed testing. View results

Mile23’s picture

Well, it does help to actually read the fail message sometimes...

1) Drupal\Tests\Component\Serialization\YamlTest::testYamlFiles with data set #40 ('/var/www/html/core/tests/Drup...es.yml')
Exception thrown parsing /var/www/html/core/tests/Drupal/Tests/Component/Scaffold/fixtures/drupal-profile/assets/profile.default.services.yml:
yaml_parse(): end of stream reached without finding document 0

/var/www/html/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php:75

So that's YamlTest discovering the scaffold test fixture files, and not being pleased with the way they are formatted. The files it found contain only a comment line.

This passes locally, which leads me to believe it's YamlPecl throwing the exception since I don't have that extension locally.

So the question is: Do we solve by adding a key to scaffold's fixture files? Or do we solve by figuring out why YamlPecl fails on valid YAML files? #3003300: PECL YAML errors at valid YAML file which only contains comments

greg.1.anderson’s picture

Ah, that is our thing then. Cool.

So the thing about the fixtures is that they use filenames that are the same as files in Drupal, but they don't necessarily have vaild contents, since we are just checking to see if the file was placed correctly. My recommendation would be to just copy a profile.default.services.yml from somewhere else. Make sure the existing comment stays in the file. That should resolve things.

greg.1.anderson’s picture

Actually profile.default.services.yml is just something we made up. It would probably be fine to just put one key `foo: bar` in here to satisfy the yml linter. Just guessing from the message, didn't actually look at the code.

Mile23’s picture

Status: Needs work » Needs review
FileSize
226.33 KB
1.37 KB

Added dummy keys to fixture files, since there's an issue for dealing with YamlPecl.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

Set to RTBC based on the following criteria:

- Tests are green
- Patch is a faithful conversion of https://github.com/grasmash/composer-scaffold
- Community members have provided a lot of feedback on aforementioned repository in the last two Composer in Core Initiative meetings. See https://www.drupal.org/project/composer_initiative/issues/3053800 and https://www.drupal.org/project/composer_initiative/issues/3051086

I think this is ready for a core committer to look at.

Mile23’s picture

Thanks. Caveats in #93.

greg.1.anderson’s picture

Oh, yeah, I forgot to comment on the phpunit 4.x issue. It would be easy enough to fix the tests to work with phpunit 4.x. There are only a couple of convenience methods used. I'd be happy to switch these over to equivalent phpunit 4.x APIs if that would make things better for core. (n.b. existing tests work fine with phpunit 5.x, which is how we tested php 5.x in the github repo.)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. This is where I've got so far. It's a lot of code so hard to keep going and I'm sorry that all these are points with something to do and not hey this looks great. Because stuff does look good and the work here is important. I will try to continue soon.
  2. +++ b/core/lib/Drupal/Component/Scaffold/AllowedPackages.php
    @@ -0,0 +1,130 @@
    +  /**
    +   * The Composer service.
    +   *
    +   * @var \Composer\Composer
    +   */
    +  protected $composer;
    +  protected $io;
    +  protected $manageOptions;
    +  protected $newPackages;
    

    Missing docs.

  3. +++ b/core/lib/Drupal/Component/Scaffold/AllowedPackages.php
    @@ -0,0 +1,130 @@
    +    // In the future, we will allow this method to add more allowed packages.
    

    That's a lot of allow in one sentence. I think this is a @todo and can be clearer about what might be done.

  4. +++ b/core/lib/Drupal/Component/Scaffold/ComposerScaffoldCommand.php
    @@ -0,0 +1,32 @@
    +/**
    + * The "composer:scaffold" command class.
    + *
    + * Composer scaffold files and generates the autoload.php file.
    + */
    +class ComposerScaffoldCommand extends BaseCommand {
    

    Potentially this could be a bit more descriptive. It took me a couple of seconds to work out you are referring to the DRUPAL_ROOT autoload.php file. For example.

  5. +++ b/core/lib/Drupal/Component/Scaffold/ComposerScaffoldCommand.php
    @@ -0,0 +1,32 @@
    +    parent::configure();
    

    Other composer commands don't call the parent here - which goes to the Symfony class and is empty. I don;t think we should either.

  6. +++ b/core/lib/Drupal/Component/Scaffold/ComposerScaffoldCommand.php
    @@ -0,0 +1,32 @@
    +    $this->setName('composer:scaffold')->setDescription('Update the Composer scaffold files.');
    

    Maybe it is worth using the setHelp() function - something like https://github.com/composer/composer/blob/522ea033a3c6e72d72954f7cd019a3...

  7. +++ b/core/lib/Drupal/Component/Scaffold/DetectAddingPackagesWithScaffolding.php
    @@ -0,0 +1,44 @@
    + * This package manages examining all required packages, and informing the
    

    This package? Is this a package or a class? Or should it be a "package listener"? Also this sentence is very long and wordy. For example, "manages examining" could be "examines". I think maybe something like:
    This package listener updates the allowed packager manager whenever a newly-required package contains scaffolding instructions. might be better.

  8. +++ b/core/lib/Drupal/Component/Scaffold/DetectAddingPackagesWithScaffolding.php
    @@ -0,0 +1,44 @@
    +class DetectAddingPackagesWithScaffolding {
    ...
    +    if (ScaffoldOptions::hasOptions($package->getExtra())) {
    ...
    +    }
    

    Looking at this some more I'm not sure we should subscribe the allowed package manager to the event and remove this class entirely - it seems to exist for the single condition which calls a static. And imo doing this can fall under the allowed package manager's responsibilities without messing up a clean architecture.

  9. +++ b/core/lib/Drupal/Component/Scaffold/DetectAddingPackagesWithScaffolding.php
    @@ -0,0 +1,44 @@
    +   *   The manager that handles allowed packages. We will inform it when new packages are added.
    

    Too long comment.

  10. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,231 @@
    + * Core class of the plugin.
    + *
    + * Contains the primary logic which determines the files to be fetched and processed.
    

    This needs improvement. And the second comment is long.

  11. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,231 @@
    +  const PRE_COMPOSER_SCAFFOLD_CMD = 'pre-composer-scaffold-cmd';
    +  const POST_COMPOSER_SCAFFOLD_CMD = 'post-composer-scaffold-cmd';
    ...
    +  protected $manageOptions;
    +  protected $manageAllowedPackages;
    +  protected $postPackageListeners;
    

    Missing docs

  12. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,231 @@
    +      $metadata = $scaffoldOpFactory->normalizeScaffoldMetadata($key, $value);
    +      $scaffoldOps[$key] = $scaffoldOpFactory->createScaffoldOp($package, $key, $metadata, $options);
    

    I think metadata normalisation belongs inside the create operation and not on the public API of the operation factory. Then the factory has one bit of public API which is the operation creation which feels pretty good for a factory class.

  13. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,231 @@
    +    $autoloadPath = ScaffoldFilePath::autoloadPath($this->rootPackageName(), $this->getWebRoot());
    +    $generator = new GenerateAutoloadReferenceFile($this->getVendorPath());
    +    $scaffoldResults[] = $generator->generateAutoload($autoloadPath);
    

    I'm not sure we need a stateful GenerateAutoloadReferenceFile class and also a static autoloadPath on the ScaffoldFilePath object. We could have a single static method on a AutoloadGenerator object that takes the $this->rootPackageName(), $this->getWebRoot() arguments and does the business and returns a ScaffoldResult object. That the autoload generation will be encapsulated in a single class.

  14. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,231 @@
    +    $manager = new ManageGitIgnore(getcwd());
    

    $manager is very generic.

  15. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationFactory.php
    @@ -0,0 +1,183 @@
    +  public function createScaffoldOp(PackageInterface $package, $dest_rel_path, $value, ScaffoldOptions $options) {
    

    Let's call this create - the class name is OperationFactory - we're creating operations.

  16. +++ b/core/lib/Drupal/Component/Scaffold/composer.json
    --- /dev/null
    +++ b/core/lib/Drupal/Component/Scaffold/phpcs.xml.dist
    
    +++ b/core/lib/Drupal/Component/Scaffold/phpcs.xml.dist
    --- /dev/null
    +++ b/core/lib/Drupal/Component/Scaffold/phpunit.xml.dist
    

    Let's not maintain these in components just yet.

greg.1.anderson’s picture

Thanks for the feedback, @alexpott. I'll start working on making those changes. Just one bit of clarification:

> Let's not maintain these in components just yet.

Was that comment intended to refer only to phpcs.xml.dist and phpunit.xml.dist? We need composer.json here for the subtree split, right?

alexpott’s picture

@greg.1.anderson yep we have composer.json in all the other components - we just have not done phpcs or phpunit config files before and this doesn't feel like the right issue to do that in.

greg.1.anderson’s picture

I only had time to make the following changes right now:

5. Removed call to parent.
8. Removed DetectAddingPackagesWithScaffolding as suggested.
12. I agree that this does not belong in the factory. I was sort of inclined to make a static class just to hold this one method, but instead I put it in the Handler class as suggested.
13. Altered as suggested. Did you want to make the vendor path also be a parameter to generateAutoload, and make the class static?
15. Renamed method to 'create' as suggested.

I will continue; here's a patch in case someone else wants to pick up some of this.

greg.1.anderson’s picture

Status: Needs work » Needs review

I accidentally added composer.lock, so you might want to ignore #107/#108, and continue reviewing #101. I'll fix it up after I make more changes. Setting to 'needs review' so that the test bot will run.

Status: Needs review » Needs work

The last submitted patch, 108: scaffold_2982684_107.patch, failed testing. View results

greg.1.anderson’s picture

I have a fix for the testUnmanagedGitIgnoreWhenGitNotAvailable failure that I will include in my next patch. I'll also provide an interdiff next time.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
213.64 KB
24.99 KB

More stuff.

1. No worries! Thanks for reviewing!
2. Added docs for these class fields, and also for any other class field that was missing docs.
3. Clarified that what was proposed here was prompting the user. This functionality is optional; we could also remove the todo.
4. Rephrased this comment to be more succinct, but kept it brief.
6. Added a brief setHelp call with some placeholder text. Might need to iterate on contents here.
7 + 9. This class went away.
11. See 2.
14. Used a more specific name.
16. Removed these, along with some supporting information in composer.json etc.

So, #6 at least is still TBD, but I am expecting there are probably going to be more comments coming per #105, so I'm going to drop of another patch here and iterate on the help text along with those.

grasmash’s picture

Thank you for converting the contrib project into a core patch @greg.1.anderson!

greg.1.anderson’s picture

@Mile23 did that in #101 - I just addressed some of the review comments.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
213.92 KB

Same as #112, but with better help text. No interdiff, because this is nearly the same as #112. See the interdiff in #112.

Setting back to RTBC to indicate that all of the review comments given so far have been addressed, and we're ready for more.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. Still going...
  2. +++ b/core/lib/Drupal/Component/Scaffold/AllowedPackages.php
    @@ -0,0 +1,160 @@
    +  /**
    +   * Called when a newly-added package is discovered to contian scaffolding instructions.
    +   */
    +  public function addedPackageWithScaffolding(PackageInterface $package) {
    +    $this->newPackages[$package->getName()] = $package;
    +  }
    

    No need for this to be public anymore. Or even really exist. We could do this in the event method with no loss of clarity.

  3. +++ b/core/lib/Drupal/Component/Scaffold/AllowedPackages.php
    @@ -0,0 +1,160 @@
    +    $jobType = $operation->getJobType();
    +    $reason = $operation->getReason();
    

    I think these lines are superfluous.

  4. +++ b/core/lib/Drupal/Component/Scaffold/ComposerScaffoldCommand.php
    @@ -0,0 +1,52 @@
    +For more information, see https://gihub.com/drupal/scaffold.
    

    I guess we shouldn't be pointing to github (or gihub). Can we put a placeholder page somewhere in the drupal docs?

  5. +++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
    @@ -0,0 +1,100 @@
    +class GenerateAutoloadReferenceFile {
    
    +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +    $generator = new GenerateAutoloadReferenceFile($this->getVendorPath());
    +    $scaffoldResults[] = $generator->generateAutoload($this->rootPackageName(), $this->getWebRoot());
    

    There's no need for GenerateAutoloadReferenceFile to be a concrete object. We could have a class called AutoloadReferenceFile which has one static method called generate which takes 3 arguments - $vendor_path, $web_root, and $root_package_name and I'm not sure that $root_package_name is actually required because ScaffoldFilePath is used unnecessarily in the object atm. Ah I see. You need it because ScaffoldResult needs it. Anyway the point still stands that GenerateAutoloadReferenceFile is stateful and exposing of API neither of which seems to be required.

  6. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +  const PRE_COMPOSER_SCAFFOLD_CMD = 'pre-composer-scaffold-cmd';
    +  const POST_COMPOSER_SCAFFOLD_CMD = 'post-composer-scaffold-cmd';
    +  /**
    +   * The Composer service.
    +   *
    +   * @var \Composer\Composer
    +   */
    +  protected $composer;
    +  /**
    +   * Composer's I/O service.
    +   *
    +   * @var \Composer\IO\IOInterface
    +   */
    +  protected $io;
    +  /**
    +   * The manager of the options in the top-level composer.json's 'extra' section.
    +   *
    +   * @var Drupal\Component\Scaffold\ManageOptions
    +   */
    +  protected $manageOptions;
    +  /**
    +   * The manager that keeps track of which packages are allowed to scaffold.
    +   *
    +   * @var Drupal\Component\Scaffold\AllowedPackages
    +   */
    +  protected $manageAllowedPackages;
    +  /**
    +   * The list of listeners that are notified after a package event.
    +   *
    +   * @var PostPackageEventListenerInterface
    +   */
    +  protected $postPackageListeners;
    

    No comments on the top constants and there's a lack of spacing.

  7. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +    $this->postPackageListeners = [];
    

    We can initialise on the property definition.

  8. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +  public function getPackageFileMappings(PackageInterface $package) {
    

    Can be protected - only called from :: getFileMappingsFromPackages() - also perhaps these tow methods should be closer in the file.

  9. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +      $metadata = $this->normalizeScaffoldMetadata($key, $value);
    +      $scaffoldOps[$key] = $scaffoldOpFactory->create($package, $key, $metadata, $options);
    ...
    +  /**
    +   * Normalize metadata, converting literal values into arrays with the same meaning.
    +   *
    +   * Conversions performed include:
    +   *   - Boolean 'false' means "skip".
    +   *   - A string menas "replace", with the string value becoming the path.
    +   *
    +   * @param string $key
    +   *   The key (destination path) for the value to normalize.
    +   * @param mixed $value
    +   *   The metadata for this operation object, which varies by operation type.
    +   *
    +   * @return array
    +   *   Normalized scaffold metadata.
    +   */
    +  protected function normalizeScaffoldMetadata($key, $value) {
    +    if (is_bool($value)) {
    +      if (!$value) {
    +        return ['mode' => 'skip'];
    +      }
    +      throw new \Exception("File mapping {$key} cannot be given the value 'true'.");
    +    }
    +    if (empty($value)) {
    +      throw new \Exception("File mapping {$key} cannot be empty.");
    +    }
    +    if (is_string($value)) {
    +      $value = ['path' => $value];
    +    }
    +    // If there is no 'mode', but there is an 'append' or a 'prepend' path,
    +    // then the mode is 'append' (append + prepend).
    +    if (!isset($value['mode']) && (isset($value['append']) || isset($value['prepend']))) {
    +      $value['mode'] = 'append';
    +    }
    +    // If there is no 'mode', then the default is 'replace'.
    +    if (!isset($value['mode'])) {
    +      $value['mode'] = 'replace';
    +    }
    +    return $value;
    +  }
    

    The normalisation of $value can occur inside the OperationsFactory - this doesn't feel like it should be the concern of the general scaffold command.

  10. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +      return;
    

    Is it worth noting to the user that scaffolding has done nothing. I think maybe we'd want this if you call the scaffold command yourself.

  11. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +    // Call any pre-scaffold scripts that may be defined.
    +    $dispatcher = new EventDispatcher($this->composer, $this->io);
    +    $dispatcher->dispatch(self::PRE_COMPOSER_SCAFFOLD_CMD);
    +    // Fetch the list of file mappings from each allowed package and
    +    // normalize them.
    +    $file_mappings = $this->getFileMappingsFromPackages($allowedPackages);
    +    // Analyze the list of file mappings, and determine which take priority.
    +    $scaffoldCollection = new OperationCollection($this->io);
    +    $locationReplacements = $this->manageOptions->getLocationReplacements();
    +    $scaffoldCollection->coalateScaffoldFiles($file_mappings, $locationReplacements);
    +    // Write the collected scaffold files to the designated location on disk.
    +    $scaffoldResults = $scaffoldCollection->processScaffoldFiles($this->manageOptions->getOptions());
    +    // Generate an autoload file in the document root that includes
    +    // the autoload.php file in the vendor directory, wherever that is.
    +    // Drupal requires this in order to easily locate relocated vendor dirs.
    +    $generator = new GenerateAutoloadReferenceFile($this->getVendorPath());
    +    $scaffoldResults[] = $generator->generateAutoload($this->rootPackageName(), $this->getWebRoot());
    +    // Add the managed scaffold files to .gitignore if applicable.
    +    $gitIgnoreManager = new ManageGitIgnore(getcwd());
    +    $gitIgnoreManager->manageIgnored($scaffoldResults, $this->manageOptions->getOptions());
    +    // Call post-scaffold scripts.
    +    $dispatcher->dispatch(self::POST_COMPOSER_SCAFFOLD_CMD);
    

    This is very dense some new lines before the comments wouldn't go amiss.

  12. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +  public function getWebRoot() {
    

    This is only publicly called from a test so should it be part of the public API?

  13. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +  public function getVendorPath() {
    

    This can be protected. No public usage.

  14. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,285 @@
    +    $filesystem = new Filesystem();
    +    $filesystem->ensureDirectoryExists($vendorDir);
    

    This is interesting. Why is it necessary to create the vendor dir - I would have thought that that is composers job.

  15. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   * @var array
    

    Is this string[]

  16. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   * SetData allows the client to associate a standard data set to use when interpolating.
    

    Too long and needs to begin with something like Allows

  17. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   * Add data allows the client to add to the standard data set to use when interpolating.
    

    Too long. Needs that active verb with an s too.

  18. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   * Interpolate replaces tokens in a string with values from an associative array.
    

    Too long and active verb with s

  19. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   * item is fetched from the array, and the token [user.name] is
    +   * replaced with the result.
    

    replaced can be on the previous line.

  20. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   *   Data to use for interpolation in addition to whatever was provided by self::setData().
    

    Too long

  21. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   *   The value to substitute for tokens that
    +   *   are not found in the data. If `false`, then missing
    +   *   tokens are not replaced.
    

    Not flowed correctly.

  22. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   * FindTokens finds all of the tokens in the provided message.
    

    As above.

  23. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   * Replacements finds the tokens that exist in a message and builds a replacement array.
    

    Too long and Finds...

  24. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,141 @@
    +   * Get a value from an array.
    

    Gets

  25. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +   * Determine whether the specified scaffold file is already ignored.
    

    Determines

  26. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +   *   Whether the specified file is already ignored or not (TRUE if ignored).
    

    This can be said with less words: TRUE if the specified file is ignored, FALSE if not.

  27. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +  public function checkIgnore($path) {
    ...
    +  public function checkTracked($path) {
    ...
    +  public function isRepository() {
    ...
    +  public function vendorCommitted() {
    

    Can be protected.

  28. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +   * Determine whether the specified scaffold file is tracked in the repository.
    

    Determines... but be careful of length of the comment.

  29. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +   * Determine whether we should manage gitignore files.
    

    Determines

  30. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +  public function managementOfGitIgnoreEnabled(ScaffoldOptions $options) {
    

    Can be protected.

  31. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +      $isIgnored = $this->checkIgnore($scaffoldResult->destination()->fullPath());
    +      $isTracked = $this->checkTracked($scaffoldResult->destination()->fullPath());
    +      if (!$isIgnored && !$isTracked && $scaffoldResult->isManaged()) {
    

    This construction results in unnecessary git operations. If $isIgnored is TRUE there's no point to checking the tracking status.

  32. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +  public function addToGitIgnore($dir, array $entries) {
    

    Can be protected

  33. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,175 @@
    +  public function gitIgnoreContents($gitIgnorePath) {
    

    No need for this to be public and changing a file in a getter is surprising - given this is only used from addToGitIgnore how about doing the \n manipulation there. There's part of me that feels this method is unnecessary and should be folded into addToGitIgnore since the behaviour of returning '' for non existent files is also surprising.

  34. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   * Get the root-level scaffold options for this project.
    

    Gets

  35. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   * @return ScaffoldOptions
    

    Should be fully qualified.

  36. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   *   The scaffold otpions object
    

    Missing fullstop

  37. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   * The scaffold options for the stipulated project.
    

    Needs a verb at the start.

  38. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   * @return ScaffoldOptions
    ...
    +   * @return Interpolator
    

    fully qualified classname

  39. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   *   The scaffold otpions object
    

    Missing full stop.

  40. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   * GetLocationReplacements creates an interpolator for the 'locations' element.
    

    Creates...

  41. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   * Ensure that all of the locatons defined in the scaffold filed exist.
    

    Ensures.

  42. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,85 @@
    +   * Create them on the filesystem if they do not.
    +   */
    ...
    +    return $locations;
    

    Missing @return

alexpott’s picture

Assigned: Unassigned » alexpott

Rather than have someone else do many of the nits from #116 I'm going to do the nitty stuff myself.

I'll un-assign myself once I've got a new patch up. I'll not touch larger architectural stuff like #116.5

alexpott’s picture

Assigned: alexpott » Unassigned
Status: Needs work » Needs review
FileSize
84.81 KB
214.43 KB

So here's a full review of nits and minor stuff in the component - I've not looked at or touched the tests yet. Only confirmed that my changes still result in a pass. I've tried to address very small architectural things where I saw them. In this initial patch we should try to keep things a protected as possible. Protected to public is easier to change than the other way around.

I didn't do #116.5 so someone else can feel free to have a go at that. Or #116.31 which I've not addressed.

We still need to look at #116.14 and .12 and .10 and .4

I will try to post more review in the coming days.

Status: Needs review » Needs work

The last submitted patch, 118: 2982684-118.patch, failed testing. View results

greg.1.anderson’s picture

Assigned: Unassigned » greg.1.anderson

Thanks @alexpott, I appreciate the review and the fixup. I'll pick up the remaining items identified so far in #116.

greg.1.anderson’s picture

Assigned: greg.1.anderson » Unassigned
Status: Needs work » Needs review
FileSize
212.61 KB
12.83 KB

4. I still need to make a placeholder page and write some preliminary docs for this.

5. Made GenerateAutoloadReferenceFile a static class.

10. Added a warning when scaffolding is not enabled for any packages.

12. Removed 'getWebroot' method entirely, as it is not needed. This had the side-effect of also removing the last unit test in the project, so we now have only integration and functional tests.

14. The `ensureDirectoryExists` on the vendor directory was overly conservative. Typically, the code that runs this check will be **inside** the vendor directory at the time the check is made, so it's sorta-kinda-pointless to take the time to create it. I removed that call.

31. Added an extra level of nesting to avoid making an unnecessary git call.

I did not confirm that the issues in #116 not called out in #118 were fixed in #118. We're ready for more feedback again -- although note that I did add one @todo, so we're not strictly speaking done. I still set this to "needs review" so that the test bot will run.

alexpott’s picture

  1. +++ b/core/lib/Drupal/Component/Scaffold/Operations/AppendOp.php
    @@ -0,0 +1,133 @@
    +  /**
    +   * Path to the source file to prepend, if any.
    +   *
    +   * @var \Drupal\Component\Scaffold\ScaffoldFilePath
    +   */
    +  protected $prepend;
    +
    +  /**
    +   * Path to the source file to append, if any.
    +   *
    +   * @var \Drupal\Component\Scaffold\ScaffoldFilePath
    +   */
    +  protected $append;
    +
    +  /**
    +   * Sets the relative path to the prepend file.
    +   *
    +   * @param \Drupal\Component\Scaffold\ScaffoldFilePath $prependPath
    +   *   The relative path to the prepend file file.
    +   *
    +   * @return $this
    +   */
    +  public function setPrependFile(ScaffoldFilePath $prependPath) {
    +    $this->prepend = $prependPath;
    +    return $this;
    +  }
    +
    +  /**
    +   * Sets the relative path to the append file.
    +   *
    +   * @param \Drupal\Component\Scaffold\ScaffoldFilePath $appendPath
    +   *   The relative path to the append file file.
    +   *
    +   * @return $this
    +   */
    +  public function setAppendFile(ScaffoldFilePath $appendPath) {
    +    $this->append = $appendPath;
    +    return $this;
    +  }
    

    So I think that operations should be immutable value objects. I.e there's no need for any setters. This could all be a constructor.

  2. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OriginalOpAwareInterface.php
    @@ -0,0 +1,36 @@
    +/**
    + * Allows operations to be informed of any operation on the same file.
    + */
    +interface OriginalOpAwareInterface {
    
    +++ b/core/lib/Drupal/Component/Scaffold/Operations/OriginalOpAwareTrait.php
    @@ -0,0 +1,42 @@
    +/**
    + * Helper trait that implements OriginalOpAwareInterface.
    + *
    + * @see \Drupal\Component\Scaffold\Operations\OriginalOpAwareInterface
    + */
    +trait OriginalOpAwareTrait {
    

    I'm not a fan of this trait and interface. I wonder if we could architect this to have a conjunction operation that joins two operations together in its process and has a constructor which takes two operations.

  3. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ReplaceOp.php
    @@ -0,0 +1,147 @@
    +  /**
    +   * The relative path to the source file.
    +   *
    +   * @var \Drupal\Component\Scaffold\ScaffoldFilePath
    +   */
    +  protected $source;
    +
    +  /**
    +   * Whether to overwrite existing files.
    +   *
    +   * @var bool
    +   */
    +  protected $overwrite;
    +
    +  /**
    +   * Sets the relative path to the source.
    +   *
    +   * @param \Drupal\Component\Scaffold\ScaffoldFilePath $sourcePath
    +   *   The relative path to the source file.
    +   *
    +   * @return $this
    +   */
    +  public function setSource(ScaffoldFilePath $sourcePath) {
    +    $this->source = $sourcePath;
    +    return $this;
    +  }
    ...
    +  /**
    +   * Sets whether the scaffold file should overwrite existing files.
    +   *
    +   * @param bool $overwrite
    +   *   Whether to overwrite existing files.
    +   *
    +   * @return $this
    +   */
    +  public function setOverwrite($overwrite) {
    +    $this->overwrite = $overwrite;
    +    return $this;
    +  }
    

    Same here

  4. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,157 @@
    +  /**
    +   * Fetches the file mappings.
    +   *
    +   * @return array
    +   *   Associative array containing package name => file mappings
    +   */
    +  public function fileMappings() {
    +    return $this->resolvedFileMappings;
    +  }
    +
    +  /**
    +   * Gets the final list of what should be scaffolded where.
    +   *
    +   * @return array
    +   *   Associative array containing destination => operation mappings
    +   */
    +  public function scaffoldList() {
    +    return $this->listOfScaffoldFiles;
    +  }
    

    These are used in tests and internally - I think we should not have them can use the properties internally.

  5. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,157 @@
    +  public function findProvidingPackage(ScaffoldFileInfo $scaffold_file) {
    

    Can be protected

  6. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,157 @@
    +  public function collateScaffoldFiles(array $file_mappings, Interpolator $locationReplacements) {
    ...
    +  public function processScaffoldFiles(ScaffoldOptions $options) {
    

    In some ways I think we could have just one method here and less state on the object. We never call one method without the other. And calling process before collate would not make any sense.

greg.1.anderson’s picture

1. I like the setters because they are more readable than constructor parameters. Immutable objects is a fine pattern, though, so I went ahead and converted the operations plus ScaffoldFileInfo and ScaffoldResult, while I was at it, to be so.

2. Yeah, that did work. Adjusted as suggested.

3. Done.

4 - 6. Makes sense, but out of time for now. TBD.

Also note that item 4 in #116 also remains undone. Making a patch in case someone else wants to pick up here. Setting to 'needs review' so that the tests will run.

greg.1.anderson’s picture

Status: Needs review » Needs work

The last submitted patch, 124: 2982684-124.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
308.7 KB

Forgot the lock file.

Mile23’s picture

FileSize
122.15 KB

121 -> 126 interdiff.

Mile23’s picture

In this Composer initiative meeting we decided to have a separate plugin/script for vendor cleanup: #3053800: Agenda for 5-15-2019

So now we also have this issue: #3057094: Add Composer vendor/ hardening plugin to core

Mile23’s picture

Added a stub documentation page which is just a copy/paste of README.md: https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...

Updated the CR with new scope (since it doesn't do vendor cleanup now): https://www.drupal.org/node/3041017

greg.1.anderson’s picture

4. Removed accessors.

5. Made the method protected.

6. Made `collateScaffoldFiles` and `processScaffoldFiles` protected, and created a new public method `process` that calls them. Removed fields $listOfScaffoldFiles and $resolvedFileMappings and had `collateScaffoldFiles` instead return their values for use in `processScaffoldFiles`.

Also resolved item 4 in #116 by pointing to the placeholder page that @Mile23 created in #129.

This resolves all review notes to date.

greg.1.anderson’s picture

Coding standards.

Status: Needs review » Needs work

The last submitted patch, 131: 2982684-131.patch, failed testing. View results

greg.1.anderson’s picture

Status: Needs work » Reviewed & tested by the community

#131 failed on the first run because space aliens; it passed after re-queueing. Ready for the next round of reviewing.

Mile23’s picture

alexpott’s picture

#131 is updating all the composer dependencies - probably for PHP7 but that shouldn't be happening here.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
106.75 KB
226.76 KB
  1. +++ b/core/lib/Drupal/Component/Scaffold/ComposerScaffoldCommand.php
    @@ -0,0 +1,52 @@
    +allowed to scaffold in the top-level composer.json will be processed
    +by this command.
    

    This text can be on the previous line by this will fit.

  2. +++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
    @@ -0,0 +1,89 @@
    +class GenerateAutoloadReferenceFile {
    

    Let's make this final and add a private constructor. That way this stays a class that only provides static methods and they are its API.

  3. +++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
    @@ -0,0 +1,89 @@
    +  public static function generateAutoload($package_name, $web_root, $vendorPath) {
    

    Missing $vendorPath documentation.

  4. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,250 @@
    +  /**
    +   * Listens to the post install command event to execute the scaffolding.
    +   *
    +   * @param \Composer\Script\Event $event
    +   *   The Composer event.
    +   */
    +  public function onPostCmdEvent(Event $event) {
    +    $this->scaffold();
    +  }
    
    +++ b/core/lib/Drupal/Component/Scaffold/Plugin.php
    @@ -0,0 +1,90 @@
    +    $this->handler->onPostCmdEvent($event);
    

    $this->handler->scaffold() and we can remove the Handler::onPostCmdEvent() - scaffold is going to stay public. Less code ftw.

  5. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,250 @@
    +    // Generate an autoload file in the document root that includes the
    +    // autoload.php file in the vendor directory, wherever that is. Drupal
    +    // requires this in order to easily locate relocated vendor dirs.
    +    $webRoot = $this->manageOptions->getOptions()->requiredLocation('web-root');
    
    +++ b/core/lib/Drupal/Component/Scaffold/ScaffoldOptions.php
    @@ -0,0 +1,243 @@
    +  /**
    +   * Returns the value of a specific named location, or throw.
    +   *
    +   * @param string $name
    +   *   The name of the location to fetch.
    +   * @param string $message
    +   *   The message to pass into the exception if the requested location
    +   *   does not exist. Optional; a default message is generated if needed.
    +   *
    +   * @return string
    +   *   The value of the provided named location
    +   */
    +  public function requiredLocation($name, $message = '') {
    +    if (!$this->hasLocation($name)) {
    +      throw new \RuntimeException($this->requiredLocationErrorMessage($name, $message));
    +    }
    +    return $this->getLocation($name);
    +  }
    +
    +  /**
    +   * Returns the error message to use when a required location was not specified.
    +   *
    +   * @param string $name
    +   *   The name of the location being fetched.
    +   * @param string $message
    +   *   The caller-provided error message, or empty if none provided.
    +   * @return string
    +   *   The error message to use when the location cannot be found.
    +   */
    +  protected function requiredLocationErrorMessage($name, $message) {
    +    if (!empty($message)) {
    +      return $message;
    +    }
    +    return "The extra.composer-scaffold.location.$name is not set in the project's composer.json.";
    +  }
    

    This seems funny to do in methods. I think this would be better to do like this.

        // Generate an autoload file in the document root that includes the
        // autoload.php file in the vendor directory, wherever that is. Drupal
        // requires this in order to easily locate relocated vendor dirs.
        $webRoot = $this->manageOptions->getOptions()->getLocation('web-root', FALSE);
        if (!$webRoot) {
          throw new \RuntimeException("The extra.composer-scaffold.location.webroot is not set in the project's composer.json.");
        }
    

    Again less API and code and easier readability.

  6. +++ b/core/lib/Drupal/Component/Scaffold/composer.json
    @@ -0,0 +1,31 @@
    +    "drupal/coder": "^8.3.2",
    

    Can remove this imo cause keeping this inline with core will be a pain.

  7. +++ b/core/lib/Drupal/Component/Scaffold/composer.json
    @@ -0,0 +1,31 @@
    +    "phpunit/phpunit": "^4.8.35 || ^6.5"
    

    phpunit 4 is no longer supported. Also we've not added this anywhere else.

  8. +++ b/core/tests/Drupal/Tests/Component/Scaffold/AssertUtilsTrait.php
    @@ -0,0 +1,20 @@
    +  /**
    +   * Asserts that a given file exists and is/is not a symlink.
    +   */
    +  protected function assertScaffoldedFile($path, $is_link, $contents_contains) {
    

    It looks worth documenting these params.

  9. Patch also contains what I was trying to get at with a ConjunctionOp that joins things together.

The patch attached does the above.

greg.1.anderson’s picture

2. 👍

4. 👍

5. 👍 Existing code was premature abstraction that is not necessary here.

6. 👍 Not sure why this was here.

+++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
@@ -0,0 +1,152 @@
+        if (isset($list_of_scaffold_files[$destination_rel_path]) && $op instanceof AppendOp) {
+          $op = new ConjunctionOp($list_of_scaffold_files[$destination_rel_path]->op(), $op);
+        }

I think it is undesirable to use `instanceof` with a concrete class instead of an interface. Even though we currently have only one operation that can override another operation, it is preferable to maintain the interface for the instanceof test, even if it is only a marker interface (with no methods). In this one respect I prefer the previous implementation. I am mostly indifferent toward the new implementation where process is called twice (once for each op) rather than calling a new method of the (formerly called) preprocess interface. The changes here are an improvement in all other ways. The renaming to ConjuctionOp is better, for example.

I am, of course, 👍 on charging forward here as presented if you disagree with my disagreement (can't use the marker interface without changing the factory, so the utility is stylistic only), so I'm not sure whether to move this to "needs work" or "rtbc". LMK what you want to do.

Mile23’s picture

I think basically this and the vendor cleanup issue are postponed on #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins so that we can have composer/composer as a dependency.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

As discussed in the Composer in Core Initiative meeting:

- This issue should not be blocked on #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins.
- My comment in #137 can be addressed as follow-on work, as it is not important.

Back to RTBC.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to derail this temporarily, but about #137: the reason why we need the instanceof AppendOp is kinda obscure (it was explained to me in Slack and to be honest I didn't quite grok it), so I think we should add a comment explaining why it's there.

greg.1.anderson’s picture

Also the marker interface I mention in #137 is necessary, because ConjunctionOp needs to implement it as well. We should also have a test demonstrating that appending on top of an append still works.

alexpott’s picture

Status: Needs work » Needs review
FileSize
70.17 KB
230.41 KB

Patch attached adds the comment @phenaproxima requested to explain the ConjunctionOp stuff. The patch also cleans up all the usages of camelCase in method parameters - it Drupal core these should be snake_case. Also gone through the tests for coding standards. I've not done much reviewing of the tests themselves (yet).

We still need to ensure we have test coverage for appending on top of an append.

greg.1.anderson’s picture

I have made a test for appending on top of an append. I was offline; I'll need to roll it in to #142.

greg.1.anderson’s picture

Here's the append-over-append test I mentioned in #143. Also, note that it was not necessary to make ConjunctionOp implement the interface, as only the new op (in our case, only the append op) needs to be marked as "conjoinable". I added the marker interface here since I feel it better style, even though it is not necessary from a functional standpoint.

I have more scaffold test cleanup to do that I will post later.

greg.1.anderson’s picture

This patch is the same as #144 with all of the same tests, except that the file ScaffoldText.php has been reorganized to be more readable / rational. I have included an interdiff, although the file changed so much that it's probably better to just read the new ScaffoldText.php and ignore the diff.

Ready for the next round of reviews.

greg.1.anderson’s picture

There were a few coding standard errors in #145. This fixes those, and also converts two tests into one with a data provider.

kim.pepper’s picture

Status: Needs review » Needs work
Issue tags: +Needs re-roll
Mixologic’s picture

Assigned: Unassigned » Mixologic

I'll take on this reroll. Should be easypeasy.

Mixologic’s picture

Rerolled to remove the composer/composer dev dependency.

Mixologic’s picture

Status: Needs work » Needs review

I think Im 0/∞ in remembering to reset the status when I upload a patch.

greg.1.anderson’s picture

Status: Needs review » Needs work

While working on an experimental drupal/drupal layout project using drupal/drupal-scaffold, I discovered two bugs in this patch:

1. If there is no "location" in the composer scaffold 'extra' section, then the destination path is defaulting to '/' instead of './'
2. The scaffold operation only runs on 'composer update' (or if explicitly called). It does not run on 'composer install', as it should.

I'll fix these bugs shortly.

greg.1.anderson’s picture

Fixed the missing 'web-root' bug. The expected result was to get an error message if no web-root was defined, but the actual behavior was a "permission denied" on a bad path. I'll make a patch with the fix after I fix the 'composer install' bug as well.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
5.78 KB
219.34 KB

Here's a patch + interdiff with both bugs mentioned in #151 fixed.

Status: Needs review » Needs work

The last submitted patch, 153: 2982684-153.patch, failed testing. View results

greg.1.anderson’s picture

Failed to add the new file to the patch in #153. Here's the same patch, but with all of its parts included.

greg.1.anderson’s picture

Status: Needs work » Needs review

... and now with the correct status too.

Mile23’s picture

Status: Needs review » Needs work

Just a minimal review from some time reading the code...

  1. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,245 @@
    +    // Fail fast if the required 'web-root' location is not set in composer.json.
    +    $webRoot = $this->manageOptions->getOptions()->getLocation('web-root');
    +    if (!$webRoot) {
    +      throw new \RuntimeException("The extra.composer-scaffold.location.web-root is not set in the project's composer.json.");
    
    +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ScaffoldTest.php
    @@ -0,0 +1,378 @@
    +    $this->expectException(\Exception::class);
    +    $this->expectExceptionMessage("The extra.composer-scaffold.location.web-root is not set in the project's composer.json.");
    

    Should expect a \RuntimeException.

  2. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,245 @@
    +      throw new \RuntimeException("The extra.composer-scaffold.location.web-root is not set in the project's composer.json.");
    

    Add the word 'configuration' before 'is'.

  3. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,156 @@
    +   * Given the list of all scaffold file info objects, return the package that
    +   * provides the scaffold file info for the scaffold file that will be placed
    +   * at the destination that this scaffold file would be placed at. Note that
    

    Nit: This sentence could be clearer...

  4. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,156 @@
    +      throw new \RuntimeException("Scaffold file not found in list of all scaffold files.");
    

    Give some context for this error... Like what file wasn't found and which package needs it.

  5. +++ b/core/lib/Drupal/Component/Scaffold/Plugin.php
    @@ -0,0 +1,91 @@
    +    // We use a separate PluginScripts object. This way we separate
    

    "We use a handler object to separate..."

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
221.64 KB
4.49 KB

Addressed comments from #157.

Regarding #157.4, this exception is logically impossible to throw; it is only included to catch errors introduced by the caller. I enhanced the information in the message anyway.

greg.1.anderson’s picture

The documentation stated that the `web-root` location defaulted to `.`, but the code required it to be set. This patch makes the code match the documentation, per @mixologix @phenaproxima & @grasmash.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed all the interdiffs, Im certainly pleased with the outcome of this, and really excited to get this into core.

greg.1.anderson’s picture

Adding @grasmash and @phenaproxima to draft commit credit.

greg.1.anderson’s picture

OK I guess only core committers can modify that field. I recommend adding @grasmash and @phenaproxima and @jhedstrom based on their work at DrupalCon Seattle. (Not sure if we can credit someone who has not commented on the issue, though.)

dww credited jhedstrom.

dww’s picture

Issue tags: -Needs re-roll

I think since I am/was a core subsystem maintainer, my account seems to have perms to update credits for core issues. It is possible to credit people who didn't directly comment on the issue.

Yes, excited to see all the progress here! This is fantastic. No time for a careful review or testing, but some of the luminaries of our community have worked on this, reviewed and tested, so I'm confident with leaving the status as RTBC.

Thanks to everyone who helped on this!
-Derek

Mile23’s picture

Issue summary: View changes

Updated CR a little bit for clarity. https://www.drupal.org/node/3041017

IS was a little behind as well.

larowlan’s picture

Issue summary: View changes
Issue tags: +Needs re-roll

This looks great, the docs are thorough, the feature-set is awesome and it is a huge step to get us to a better place for composer tooling, especially the feature that consideration for people who used a tarball moving to composer. Sorry the review took so long, its a big patch

  1. +++ b/core/lib/Drupal/Component/Scaffold/AllowedPackages.php
    @@ -0,0 +1,163 @@
    +    $allowed_packages = $this->evaluateNewPackages($allowed_packages);
    +    return $allowed_packages;
    

    nit: we could just return here without the assignment

  2. +++ b/core/lib/Drupal/Component/Scaffold/AllowedPackages.php
    @@ -0,0 +1,163 @@
    +    // @todo We could prompt the user and ask if they wish to allow a newly-added package.
    

    nit: > 80

  3. +++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
    @@ -0,0 +1,97 @@
    +  public static function generateAutoload($package_name, $web_root, $vendor) {
    

    should we check the file doesn't exist before we write it? Is it possible folks might have added some modifications and/or have this in their version control?

  4. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,240 @@
    +    $scaffoldOpFactory = new OperationFactory($this->composer);
    +    $scaffoldOps = [];
    +    foreach ($package_file_mappings as $dest_rel_path => $metadata) {
    

    we have a mix of camel and snake case here - should we be consistent?

  5. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,240 @@
    +      $package_file_mappings = $this->getPackageFileMappings($package);
    +      $file_mappings[$package_name] = $package_file_mappings;
    

    nit: do we need the intermediate variable here?

  6. +++ b/core/lib/Drupal/Component/Scaffold/Handler.php
    @@ -0,0 +1,240 @@
    +    else {
    

    nit: this else isn't needed, we return above

  7. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,155 @@
    + * Inject config values from an associative array into a string.
    

    nit: normally this should start with a verb, e.g. Defines or in this case, probably Injects

  8. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,155 @@
    +class Interpolator {
    

    Is there anything we can re-use here in our existing Token code? Should we be creating a token component and making the core utilities extend from that? Happy for that to be a follow-up if so

  9. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,155 @@
    +   *   Interpolation data to use when interpolating.
    ...
    +   *   Interpolation data to use when interpolating.
    ...
    +    $this->data = array_merge($this->data, $data);
    ...
    +    $data = $extra + $this->data;
    

    should we be documenting that these values are an associative array, i.e the keys are significant?

  10. +++ b/core/lib/Drupal/Component/Scaffold/Interpolator.php
    @@ -0,0 +1,155 @@
    +      $replacementText = array_key_exists($key, $data) ? $data[$key] : $default;
    

    Now we're no longer targeting old versions of php - should we be using the null coalesce operator here instead?

  11. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,171 @@
    +      return;
    ...
    +      $this->addToGitIgnore($dir, $entries);
    

    should we document the return value and delineate between an operation that did something and one that did nothing?

  12. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,171 @@
    +      $isIgnored = $this->checkIgnore($scaffoldResult->destination()->fullPath());
    ...
    +        $isTracked = $this->checkTracked($scaffoldResult->destination()->fullPath());
    ...
    +          $path = $scaffoldResult->destination()->fullPath();
    

    should we have a local variable here to avoid calling this three times?

  13. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,171 @@
    +    $process = new Process('git check-ignore ' . $path, $this->dir);
    ...
    +    $process = new Process('git ls-files --error-unmatch ' . $path, $this->dir);
    ...
    +    $process = new Process('git rev-parse --show-toplevel', $this->dir);
    

    According to the symfony docs we should be passing these arguments using array syntax to allow symfony/process to deal with irregularities between different flavours of shells - is there a reason why we don't?

  14. +++ b/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php
    @@ -0,0 +1,171 @@
    +    $isIgnored = $process->getExitCode() == 0;
    +    return $isIgnored;
    ...
    +    $isTracked = $process->getExitCode() == 0;
    +    return $isTracked;
    ...
    +    $isRepository = $process->getExitCode() == 0;
    +    return $isRepository;
    

    nit: do we need the intermediate variables here?

  15. +++ b/core/lib/Drupal/Component/Scaffold/ManageOptions.php
    @@ -0,0 +1,90 @@
    +   * Ensures that all of the locations defined in the scaffold filed exist.
    

    should this be files and not filed?

  16. +++ b/core/lib/Drupal/Component/Scaffold/Operations/AppendOp.php
    @@ -0,0 +1,79 @@
    +    $destination_path = $destination->fullPath();
    ...
    +    $prependContents = '';
    ...
    +      $originalContents = file_get_contents($destination_path);
    

    nit: mix of snake and camel case here too, should we standarize? - the patch has a mix of both throughout

  17. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ConjoinableInterface.php
    @@ -0,0 +1,15 @@
    + * The only conjoinable operation at the moment is the "append" operation.
    

    Are we sure we want to add this comment? People can work out what implementations there are - it feels like something that could get out of alignment quickly

  18. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ConjunctionOp.php
    @@ -0,0 +1,51 @@
    +   * TYe first operation.
    

    TYe » The?

  19. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,155 @@
    +      throw new \RuntimeException("Scaffold file $dest_rel_path not found in list of all scaffold files.");
    

    in some instances we use the interpolation for this, but here we don't? is there a reason why we mix the two

  20. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,155 @@
    +    list($list_of_scaffold_files, $resolved_file_mappings) = $this->collateScaffoldFiles($file_mappings, $location_replacements);
    ...
    +   *   A list containing two lists:
    +   *     - Associative array containing destination => operation mappings.
    +   *     - Associative array containing package name => file mappings.
    

    anytime we have to do a multi-value return, its normally an indication that there's a value object we're missing - is that the case here?

  21. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,155 @@
    +        // run, one after the other. At the moment, only AppendOp is conjoinable;
    

    nit, > 80

  22. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationFactory.php
    @@ -0,0 +1,185 @@
    +      case 'skip':
    ...
    +      case 'replace':
    ...
    +      case 'append':
    ...
    +    $metadata += ['overwrite' => TRUE];
    

    do these warrant constants?

  23. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationFactory.php
    @@ -0,0 +1,185 @@
    +    $op = new ReplaceOp($source, $metadata['overwrite']);
    ...
    +    if (isset($metadata['prepend'])) {
    ...
    +    if (isset($metadata['append'])) {
    ...
    +        return ['mode' => 'skip'];
    ...
    +      $value = ['path' => $value];
    ...
    +      $value['mode'] = 'replace';
    

    In most cases we're using value objects, but we're modelling $metadata with an associative array - is there a reason?

  24. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationFactory.php
    @@ -0,0 +1,185 @@
    +    else {
    

    nit: this else isn't needed

  25. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ReplaceOp.php
    @@ -0,0 +1,115 @@
    +    if ($options->symlink() == TRUE) {
    

    nit: does the == TRUE add anything here?

  26. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ReplaceOp.php
    @@ -0,0 +1,115 @@
    +    $success = copy($this->source->fullPath(), $destination->fullPath());
    ...
    +      $fs = new Filesystem();
    +      $fs->relativeSymlink($this->source->fullPath(), $destination->fullPath());
    

    we're mixing native php file operations and symfony here - any reason not to use symfony/filesystem's copy to bypass vagueries of various OS's and stream wrappers?

  27. +++ b/core/lib/Drupal/Component/Scaffold/Plugin.php
    @@ -0,0 +1,91 @@
    +    return ['Composer\Plugin\Capability\CommandProvider' => ScaffoldCommandProvider::class];
    

    any reason not to use ::class constant here?

  28. +++ b/core/lib/Drupal/Component/Scaffold/README.md
    @@ -0,0 +1,477 @@
    +project's composer.json file. Additional configuration from the composer.jon
    

    jon » json

  29. +++ b/core/lib/Drupal/Component/Scaffold/README.md
    @@ -0,0 +1,477 @@
    +      "allowed-packages": [
    +        "drupal/core",
    +      ],
    

    ❤️ - these docs are awesome 💪

  30. +++ b/core/lib/Drupal/Component/Scaffold/ScaffoldFileInfo.php
    @@ -0,0 +1,134 @@
    +  protected function getInterpolator() {
    

    there is only one usage of this - any reason not to inline it in the calling code?

  31. +++ b/core/tests/Drupal/Tests/Component/Scaffold/AssertUtilsTrait.php
    @@ -0,0 +1,27 @@
    +    $this->assertRegExp($contents_contains, basename($path) . ': ' . $contents);
    

    any reason not to use phpunit's stringContains here? looks like that's what we're using it for anyway, none of the regex appear to be anything other than plain strings

  32. +++ b/core/tests/Drupal/Tests/Component/Scaffold/ExecTrait.php
    @@ -0,0 +1,36 @@
    +   * @return array
    +   *   Standard output and standard error from the command
    ...
    +    return [$process->getOutput(), $process->getErrorOutput()];
    
    +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ComposerHookTest.php
    @@ -0,0 +1,152 @@
    +    list($stdout,) = $this->execComposer("require --no-ansi --no-interaction fixtures/scaffold-override-fixture:dev-master", $sut);
    

    should we just return the process and leave fetching the stdout and stderr to the caller? gives them more options? multi value array returns are not pretty

  33. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Fixtures.php
    @@ -0,0 +1,368 @@
    +  public function tearDown() {
    +    // Remove any temporary directories that were created.
    +    $filesystem = new Filesystem();
    +    foreach ($this->tmpDirs as $dir) {
    

    any reason we can't use vfsstream here instead of generating actual directories? Realise that's a big pivot so happy if that is a follow-up to explore feasibility.

  34. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Fixtures.php
    @@ -0,0 +1,368 @@
    +    $replacements += ['SYMLINK' => 'true'];
    

    is it worth documenting that this is a string rather than a boolean because its being replaced with interpolation?

  35. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ComposerHookTest.php
    @@ -0,0 +1,152 @@
    +    $is_link = FALSE;
    +    $replacements = ['SYMLINK' => $is_link ? 'true' : 'false', 'PROJECT_ROOT' => $this->projectRoot];
    

    why do we hard-code $is_link to FALSE but then have logic for it being in different states? can't we just hard-code that too?

  36. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ComposerHookTest.php
    @@ -0,0 +1,152 @@
    +    @unlink($sut . '/sites/default/default.settings.php');
    ...
    +    @unlink($sut . '/sites/default/default.settings.php');
    ...
    +    @unlink($sut . '/sites/default/default.settings.php');
    

    should we assert this operation succeeded using !file_exists before we re-run to avoid a false positive? i.e. we think we deleted the file in the test and that it was re-added with the next run, but in fact, the first one was not removed.

  37. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ComposerHookTest.php
    @@ -0,0 +1,152 @@
    +    // 'composer composer:scaffold' and see if the scaffold file is re-scaffolded.
    

    nit > 80

  38. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ComposerHookTest.php
    @@ -0,0 +1,152 @@
    +  protected function execComposer($cmd, $cwd, array $env = []) {
    +    return $this->mustExec("composer {$cmd}", $cwd, $env);
    

    is this wrapper needed?

  39. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ScaffoldTest.php
    @@ -0,0 +1,364 @@
    +  public function scaffoldSut($fixture_name, $is_link = FALSE, $relocated_docroot = TRUE) {
    ...
    +    return [$docroot, $scaffoldOutput];
    

    this return is not documented, and also, a multi-value return always feels like a code smell - should we have a value object here?

  40. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ScaffoldTest.php
    @@ -0,0 +1,364 @@
    +   * Asserts that the drupal/assets scaffold files correct for drupal/project and drupal/drupal.
    

    nit: >80 here

  41. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ScaffoldTest.php
    @@ -0,0 +1,364 @@
    +        '#in drupal-drupal-test-append composer.json fixture.*This content is prepended to the top of the existing robots.txt fixture.*Test version of robots.txt from drupal/core.*This content is appended to the bottom of the existing robots.txt fixture.*in drupal-drupal-test-append composer.json fixture#ms',
    ...
    +        '#in drupal-drupal-append-to-append composer.json fixture.*This content is prepended to the top of the existing robots.txt fixture.*Test version of robots.txt from drupal/core.*This content is appended to the bottom of the existing robots.txt fixture.*in profile-with-append composer.json fixture.*This content is appended to the bottom of the existing robots.txt fixture.*in drupal-drupal-append-to-append composer.json fixture#ms',
    

    oh, here is one using the regex format. Should we just split into an array of expected strings instead of overloading with regex? - it will make future debugging easier when the assert will fail with the missing line instead of the blob of multiple lines

  42. +++ b/core/tests/Drupal/Tests/Component/Scaffold/Functional/ScaffoldTest.php
    @@ -0,0 +1,364 @@
    +    $from_core = '#from drupal/core#';
    ...
    +    $this->assertScaffoldedFile($docroot . '/.csslintrc', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/.editorconfig', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/.eslintignore', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/.eslintrc.json', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/.gitattributes', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/.ht.router.php', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/sites/default/default.services.yml', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/sites/example.settings.local.php', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/sites/example.sites.php', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/index.php', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/update.php', $is_link, $from_core);
    +    $this->assertScaffoldedFile($docroot . '/web.config', $is_link, $from_core);
    

    should we also be checking for the specific content in the file, e.g. they all contain 'from drupal/core' but we're not really checking that the correct file was copied if we don't vary the expected text - we should be able to build a unique expected string as each file also contains the file name as well as 'from drupal/core'

larowlan’s picture

Issue tags: -Needs re-roll

cross-post

dww’s picture

Assigned: Mixologic » Unassigned
Status: Reviewed & tested by the community » Needs work

Fixing status from x-post based on the 42 points from #166. ;) Thanks for the thorough review!

Not sure that @Mixologic is going to work on addressing #166, so unassigning for now.

Cheers,
-Derek

jibran’s picture

+++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
@@ -0,0 +1,97 @@
+    $fs = new SymfonyFilesystem();
+    $relative_vendor_path = $fs->makePathRelative($vendor, realpath($location));
+    $fs->dumpFile($autoload_path->fullPath(), static::autoLoadContents($relative_vendor_path));

I don't see a reason why Drupal FileSystem can't be used to achieve the same result.

Chi’s picture

I don't see a reason why Drupal FileSystem can't be used to achieve the same result.

I suppose it is not reliable to access Drupal container in that script.

greg.1.anderson’s picture

Assigned: Unassigned » greg.1.anderson

I will address the issues in #166, probably by end-of-day.

#166.3: I don't see any reason why anyone would customize this file, and I think it would probably be a bad idea. However, it's possible today, and something that's possible might likely be done. I'll change the code so that it skips rewriting the autoload.php file if it exists and has been added to the repository.

#166.8 / #169: I think it's best to avoid using code from the project (in this case Drupal) from within a Composer plugin. While technically possible, bad things can happen, e.g. if you call some Drupal code prior to `composer install`, or if code changes out from under the plugin during a `composer update`. The later was the cause of an inconvenient upgrade bug in drupal-composer/drupal-project in the past.

#166.10: We were using a bunch of PHP 7.0+ features in this project in an earlier incarnation, but had to roll them back because it's my understanding that it's still core policy to code to PHP 5.6+ to make backporting easier.

#166.19: This exception is logically impossible to ever be reached (unless the code in the caller grows a bug), so I guess the answer is "laziness". Originally this exception didn't have any variable references at all. I can fix it up for consistency anyway.

#166.20: Yes, a value object would be better here. However, this method is only used internally to the class, so it seemed like too much effort to break out the code for a class file that's already pretty short. I won't do this refactor unless someone feels strongly that it should be done.

#166.23: Metadata comes to us from Composer `extra` as an associative array. We could make a value object wrapper for this if it was felt to be necessary.

#166.33: Scaffolding involves creating a lot of files. I'm not sure there's a convenient way to use vfsstream for these tests.

#166.34 / 35: I am tempted to remove symlink as a replacement and just duplicate any fixture where both the symlink and non-symlink variants are used. In some cases I used variable names for some things that are hardcoded because I hadn't yet decided if each given test needed a data provider to improve coverage. At this point I think the coverage is pretty good; your opinion on that prior to simplification of the code would be appreciated vis-a-vis symlink permutation coverage. It should work about the same way for most of these tests, so I tend to think we don't need to spend the time to do more permutations than already exist.

#166.39: I agree that a value object would be better here too, but I tend to be more casual with the test fixture code. I'll make a value object if you think it would be better.

The rest I will address as suggested, unless I run into some issue when I'm fixing that I didn't consider when skimming the suggestions.

Thanks for the thorough review - I appreciate it. Please do not feel shy about asking me to do any of the things ^^ that I think are not-so-important if you really do think they are important.

greg.1.anderson’s picture

@larowlan: I'd also like to suggest that @phenaproxima, @grasmash and @jheadstrom be re-added to this list of folks receiving commit credit here. Although they do not have any patches on this issue, they all spent > 1 day at DrupalCon Seattle working on the code that went into this patch.

phenaproxima’s picture

Attempting to add credit (not sure if I can do that, but I'll give it a shot).

dww’s picture

Re: #172: @grasmash, @jhedstrom and @phenaproxima all have credit on this issue now (since #164), unless a core committer undoes the change. Their checkboxes are selected by default when I view the form element. All this code is weird, and I don't fully understand all the edge cases for the behavior. *shrug*

Re: #171: mostly agree with all you've written. However, for #166.10, I don't see the core committers back-porting this to 8.7.x and earlier. I suspect that's too big a change inside a "stable" release series. So I don't see any reason to support PHP 5.6 since 8.8 already forces PHP7. Release manager input would obviously override this, but I suspect this is *not* going to be backported. Hopefully I'm wrong. ;)

Cheers,
-Derek

dww’s picture

Issue summary: View changes

p.s. @Mile23: can you confirm if https://www.drupal.org/node/2982684/revisions/view/11467471/11467626 was an accident? Seems like @larowlan in #166 undid your changes from #165. Tentatively restoring those fixes with this comment. Please re-fix if I've messed anything up.

Thanks,
-Derek

Mile23’s picture

Re #175

Yes, that looks like an accident. Thanks for fixing it.

greg.1.anderson’s picture

@dww: I understand your reasoning regarding #166.10, but please see the comments in https://github.com/grasmash/composer-scaffold/issues/36#issuecomment-490.... I don't want to go back and forth on PHP 7.0 support. Since we already took it out once, let's make further adjustments as a follow-on issue.

#174: Commit credit was added in #164, removed in #166, and re-added in #173. I think it is now 👍

#166.13: I tried to make this change, and made a shocking discovery. Since the scaffold tool runs as a Composer plugin, any class that Composer uses before our plugin is called will continue to be used by our code. Composer 1.8.5 uses Symfony/Process 2.8.48. This means that we cannot use the array variant of the Process constructor, because is is not supported in Symfony/Process 2.8. The other implication of this is that when a new version of Composer comes out that uses a newer version of Symfony/Process, then we'll spontaneously start using the newer API, regardless of what is in our lock file. Because of this, I am wondering if we should remove all calls to Symfony/Process (and potentially other Symfony componets as well) from the scaffold plugin.

greg.1.anderson’s picture

Per @phenaproxima, we should use ProcessExecutor and Filesystem from the Composer API here instead.

This is a good solution, so I'm going to move forward with it throughout the plugin.

greg.1.anderson’s picture

#166.20: I am refactoring the code to avoid the multiple return values. This is in progress.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
42.59 KB
222.88 KB

This is taking me a while, so I'm posting a progress patch.

Addresses some of the review comments from #166:

- 1. Simplified
- 2. Wrapped
- 3. Added check to see if autoload file committed to repo
- 4. Converted all local variables that were camelCase into snake_case
- 5. Removed intermediate variable
- 6. Removed 'else'
- 7. Improved comment
- 8. No change; not safe to use Drupal code from a Composer plugin
- 9. Improved phpdocs
- 10. No change; not using PHP 7+ yet
- 11. Not sure what to do here; caller does not act on return value.
Could potentially print some progress output when managing .gitignore
- 12. Used local variable to store accessor result
- 13. Converted to use Composer Filesystem instead of Symfony Filesystem for
safetly / stability. Composer Filesystem does not support array syntax.
- 14. Removed unnecessary intermediate variables
- 15. Fixed typo
- 16. Changed camelCase into snake_case
- 17. Removed unnecessary comment
- 18. Fixed typo
- 19. TODO: No change yet; didn't want to instantiate an interpolator for
an exception that will never be thrown. Maybe this should be deleted?
- 20. TODO: Not sure how to resolve; still considering.
- 21. Fixed wrap
- 22. Added constants...
- 23. ... and renamed 'metadata' to OperationData and created wrapper object for it
- 24. Removed unnecessary 'else'
- 25. Remove redundant `== TRUE`
- 26. Use Composer::Filesystem instead
- 27. Used ::class constant
- 28. Fixed typo.
- 29. Thanks!
- 30. Inlined getInterpolator() method

Still need to do the tests, and a couple of the items as noted above. Setting to 'needs review' just to ensure that the tests run.

Mile23’s picture

13. Converted to use Composer Filesystem instead of Symfony Filesystem for
safetly / stability. Composer Filesystem does not support array syntax.

You mean composer's ProcessExecutor?

Re: #177:

Composer 1.8.5 uses Symfony/Process 2.8.48.

You're referring to the version baked into the composer.phar file... Interesting.

greg.1.anderson’s picture

@Mile23 Yes that was a typo. I did mean Composer's ProcessExecutor. I used Composer's Filesystem elsewhere as well.

greg.1.anderson’s picture

Progress patch:

#166.19: The scaffold file parameter already has an interpolator, so this suggestion was actually trivial once I noticed that. (doh)
#166.20: I was reluctant to make a class just to hold two values; however, I discovered that if I factored out the responsibility for collating the scaffold files into a separate class, then the second value could remain protected in that class, and did not need to be exposed to the caller at all.

Still need to address review comments on the tests (#166.32-42)

yogeshmpawar’s picture

yogeshmpawar’s picture

Few coding standard issues fixed & added an interdiff between patches.

Status: Needs review » Needs work

The last submitted patch, 185: 2982684-185.patch, failed testing. View results

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
24.83 KB
84.82 KB
225.68 KB

@yogeshmpawar, thank you for taking the time to contribute to this patch. I'm skipping #185 for now because it's not clear to me why it is failing, and I don't have time to fix it this morning. if you would like to re-apply your changes to this patch, that would be appreciated. You can test locally via:

./vendor/bin/phpunit -c core --group Scaffold

If not, I'll see if I can circle back and integrate #185 later.

This patch addresses the rest of the comments from #166 (but not #185):

- 31. Replaced all regex asserts with simple 'assertContains'
- 32. None of the tests assert on stderr, so I changed ExecTrait to return only stdout. I did not want to return the process object as I want to shield the tests from which process runner the trait is using.
- 33. vfsstream is more flexible than I had imagined it to be; I think it could work well for most of the tests here. However, the Composer hook tests could not use vfsstream, I think, because these tests `exec` the Composer binary. Perhaps some future investigation could be done into using vfsstream. It might be possible to rewrite the hook tests without exec.
- 34. Noted why 'true' must be a string rather than a boolean.
- 35. Code such as `$is_link = false` was placeholders for eventual dataproviders, if needed. Replaced these where they appeared.
- 36. Added assertFileNotExists after unlink
- 37. Wrapped to < 80
- 38. Removed thin wrapper 'execComposer'. This was done initially to maintain alignment between the 'exec' tests and the 'runComposer' tests (that call the Composer API) so that test snippets could be copied between them more easily. This isn't such a concern any more, and in any event, adding or removing the word 'composer' is not hard.
- 39. Wasn't sure if a larger refactor was warranted here (similar to reoslution of #166.20), but settled for making a simple value wrapper.
- 40. Fixed > 80
- 41. Same as 31.
- 42. Made scaffold file asserts more specific

greg.1.anderson’s picture

Re-applying improvements from #185. Thanks, @yogeshmpawar.

Mixologic’s picture

+++ b/core/lib/Drupal/Component/Scaffold/README.md
@@ -0,0 +1,487 @@
+The scaffold tool automatically creates the required `autoload.php` file at the ¶
+Drupal root as part of the scaffolding operation. This file should not be ¶

Trivial extra whitespace at the end of these lines caused patch application to moan.

borisson_’s picture

Status: Needs review » Needs work

I didn't look at the tests in detail, but I went trough the main part of the patch and I found some nitpicks. Overall this has really good documentation and looks really good.

  1. +++ b/core/lib/Drupal/Component/Scaffold/AllowedPackages.php
    @@ -0,0 +1,163 @@
    +      if ($package && $package instanceof PackageInterface && !array_key_exists($name, $allowed_packages)) {
    

    the if ($package) here doesn't add anthing since the return value can either be null or something of PackageInterface.
    I also personally prefer to have positive assertions where possible.
    So I think something like this is better:

    if ($page instaceof PackageInterface && isset($allowed_packages[$name])

  2. +++ b/core/lib/Drupal/Component/Scaffold/AllowedPackages.php
    @@ -0,0 +1,163 @@
    +    // @todo We could prompt the user and ask if they wish to allow a
    +    // newly-added package.
    

    Before we get this in, we should only have @todos with a link to a drupal.org issue. So we should create one for this and link to it from here.

    A normal todo without a link to d.o will either lead to some duplicate issues or the issue being forgotten.

  3. +++ b/core/lib/Drupal/Component/Scaffold/ComposerScaffoldCommand.php
    @@ -0,0 +1,52 @@
    +For more information, see https://www.drupal.org/docs/develop/using-composer/using-drupals-composer-scaffold.
    +EOT
    

    Should we wrap this under 80 cols? Or will that lead to a blank line in the output as well.

  4. +++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
    @@ -0,0 +1,121 @@
    +  private function __construct() {
    +  }
    

    This is the first time we introduce a private constructor in core. I like this - but it already a really big patch, should we use that to also introduce a new coding style as well?

    I like this a lot - so we can probably keep this.

  5. +++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
    @@ -0,0 +1,121 @@
    +   * @return bool
    +   *   True if autoload.php file exists and has been committed to the repository
    

    I think there needs to be an empty line before an @return

  6. +++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
    @@ -0,0 +1,121 @@
    +      return false;
    

    Should be FALSE in caps.

  7. +++ b/core/lib/Drupal/Component/Scaffold/Operations/AppendOp.php
    @@ -0,0 +1,84 @@
    +      // Assume that none of these files is very large, so load them all into
    +      // memory for now. Considering uses streams to scaffold large files.
    

    Again, should this be an @todo with a link to d.o?

  8. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationFactory.php
    @@ -0,0 +1,134 @@
    +class OperationFactory {
    +  /**
    

    Needs an empty line.

  9. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationFactory.php
    @@ -0,0 +1,134 @@
    +      case 'replace':
    +        return $this->createReplaceOp($package, $operation_data);
    

    Can this be ReplaceOp::ID?

  10. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ReplaceOp.php
    @@ -0,0 +1,121 @@
    +   * Copy or Symlink the specified scaffold file.
    +   *
    +   * {@inheritdoc}
    

    Currently it is not allowed according to our documentation standards to have both {@inheritdoc} AND something else in the same docblock. There is an issue to change that: https://www.drupal.org/project/coding_standards/issues/1994890

    In this case, we either copy the entire doc or remove the short description from here.

  11. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ScaffoldFileCollator.php
    @@ -0,0 +1,118 @@
    +   * Collation of all scaffold files. The top level array maps from the
    +   * package name to the collection of scaffold files provided by that
    +   * package. Each collection of scaffold files is keyed by destination path.
    

    the title should be only one line, with additional documentation seperated by a blank line.

    /**
     * title max 80 cols.
     *
     * more
     * docs
     *
     * @var ..
     */
    
  12. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ScaffoldFileCollator.php
    @@ -0,0 +1,118 @@
    +   * Collection of all destination paths to be scaffolded to the last file
    

    ^

  13. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ScaffoldFileCollator.php
    @@ -0,0 +1,118 @@
    +  public function resolvedScaffoldFiles() {
    

    Needs docs.

  14. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ScaffoldFileCollator.php
    @@ -0,0 +1,118 @@
    +  public function overridden(ScaffoldFileInfo $scaffold_file) {
    

    Needs docs.

  15. +++ b/core/lib/Drupal/Component/Scaffold/Plugin.php
    @@ -0,0 +1,92 @@
    +class Plugin implements PluginInterface, EventSubscriberInterface, Capable {
    +  /**
    

    Needs a blank line after class declaration.

  16. +++ b/core/lib/Drupal/Component/Scaffold/README.md
    @@ -0,0 +1,487 @@
    +# composer-scaffold
    

    This documentation is awesome! Thank you so much.

greg.1.anderson’s picture

Thanks for the feedback, @borisson_. I addressed your comments as follows:

- 1. Simplified 'if' a bit; however, `isset` and `array_key_exists` work about the same way, so the negation is still needed in the expression in order to maintain its current meaning.
- 2. Added reference to an issue: https://www.drupal.org/project/drupal/issues/3064990. This issue could use some reworking to follow drupal.org conventions.
- 3. The URL on its own is >80 col. Unless we care to make a shorter URL, we cannot get this block down to < 80 columns on every line, so I left it as-is.
- 4. The private constructor is appropriate here, but this is an internal class, so the impact of removing it would be minimal. Still, I will leave this in unless someone decides it must be removed.
- 5. Added blank line.
- 6. Corrected FALSE
- 7. There is no requirement for large scaffold files, so it seems unnecessary to create an issue. Instead, I removed the suggestion.
- 8. Added blank line.
- 9. Used constant.
- 10. The additional comment didn't add anything over the comment at the top of the file, so I just removed it.
- 11 & 12. Corrected formatting of comment blocks.
- 13 & 14. Added docs.
- 15. Added blank line.

greg.1.anderson’s picture

Status: Needs work » Needs review

Forgot to set to 'needs review' for the test bot.

greg.1.anderson’s picture

FileSize
226.51 KB

in #191 I accidentally saved the interdiff as the .patch file. Here is a corrected patch. For the interdiff, see 189-to-191-interdiff.txt

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I don't see anything else that needs to be changed or even could be changed to make this more ready for inclusion into core.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 193: 2982684-193.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

greg.1.anderson’s picture

Well, now I'm embarrassed. I made a minor change to the wording of one of the error messages, and did not update the test to match. Here's a new patch to fix it (along with more appealing shade of blue on the message text).

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

PostgreSQL and SQLite tests will probably fail due to #3059332-33: Mark kernel tests that perform no assertions as risky

I'm going to RTBC here because it LGTM. :-) But then I'm going to start up some testbot runs for PHP 7.0 and 7.2.

The next step is #3057094: Add Composer vendor/ hardening plugin to core, so we can move on to #2982680: Add composer-ready project templates to Drupal core

Mile23’s picture

It looks like the PHP 7.0 build failed with this:

17:49:53 yarn run v1.3.2
17:49:54 $ cross-env BABEL_ENV=development node -r dotenv-safe/config -r babel-register ./node_modules/.bin/nightwatch --config ./tests/Drupal/Nightwatch/nightwatch.conf.js
17:49:54 /bin/sh: 1: cross-env: not found
17:49:54 error Command failed with exit code 127.
17:49:54 info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
17:49:54 ---------------- nightwatchjs Returned 1 ----------------

Trying again.

Mixologic’s picture

7.0.x-dev is probably not the environment you wanted to run. (didnt realize that was even available)

Mile23’s picture

Restarting the sqlite test after #3059332-42: Mark kernel tests that perform no assertions as risky

Also I just now learned that apparently we won't be supporting PHP 7.0 in core 8.8.x, so therefore the failed PHP 7.0 builds in #196 aren't an issue.

Mile23’s picture

OK, so PHP 7@stable is supported for 8.8.x, so I started that test on #196.

borisson_’s picture

All envs are green, so that's great! There are 3 new coding standards issues introduced in the latest patch (they are mentioned on https://www.drupal.org/pift-ci-job/1335751), we should fix those as well. Not pushing this back to rtbc for just that though.

Mile23’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
226.55 KB
1.53 KB

Fixes CS. Docblock changes only.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC from #197.

greg.1.anderson’s picture

I reviewed the interdiff and patch files one more time, and also believe this is rtbc now.

Mixologic’s picture

So very reviewed, much tested. +1

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
24.69 KB
219.86 KB

Some thoughts that have been bothering me for a bit.

  1. +++ b/core/lib/Drupal/Component/Scaffold/Operations/OperationCollection.php
    @@ -0,0 +1,79 @@
    +/**
    + * Manages and processes the collection of files to be scaffolded.
    + */
    +class OperationCollection {
    

    This is not really a collection. It's a processor. Or a collection processor.

  2. +++ b/core/lib/Drupal/Component/Scaffold/Operations/ScaffoldFileCollator.php
    @@ -0,0 +1,139 @@
    +/**
    + * Collates the scaffold file for the OperationCollection class.
    + */
    +class ScaffoldFileCollator {
    

    This could be a collection and we could extend from Iterator and be something we could loop around and maybe hide away some of these arrays that we're passing around.

  3. I been staring at the code for sometime trying to work out the best way to implement the above and I was wondering why we bother with maintaining both \Drupal\Component\Scaffold\Operations\ScaffoldFileCollator::$resolvedFileMappings and $listOfScaffoldFiles and how the whole override thing works.
  4. I've now spent even more time staring at the code and I think the original operations that being added to conjunctions are also still being run (because we never unset them)

Sorry for setting this back to needs review but maybe someone who is more adept at iterators can think of an even better way of doing this but I think the attached patch fixes a couple of subtle bugs and results in more reliable code as now \Drupal\Component\Scaffold\Operations\ScaffoldFileCollection is a static iterator generator with no state to keep in sync and the whole override thing has been replaced by the existing SkipOp.

Ghost of Drupal Past’s picture

If someone slacks me with info about what iterator problems we have here I might be able to help, reading such a huge enough patch to comprehend is a bit beyond my free time right now. A tiny note, not worth keeping up the patch for but if it's rerolled: return isset($this->data[self::OVERWRITE]) ? $this->data[self::OVERWRITE] : TRUE; no need for that any more $this->data[self::OVERWRITE] ?? TRUE.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

The comment in #166 point 10 spawned a discussion in slack whether or not using the NULL Coalesce operator was okay or not, as its among the "php7 only features". And while this patch is adding entirely new code, we have to be careful about php7isms, particularly if our phpcs/dev process doesnt *yet* support them or know what to do with them.

What we found out is that core already includes one instance of a Null Coalesce operator (https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor...), and phpcs didnt raise any flags on that patch, ergo Null Coalesce is fair game, that being said, however, there was initially an abundance of caution to not make the patch keep flip flopping due to the grey area that is "Drupal 8.8's allowable php7isms", so its not surprising that there are instances of ternary isset's checking for nulls.

I don't think we have any 'iterator problems' per se.

@alexpott: I gave your changes a pretty thorough examination, and yay, less code doing what Im somewhat mostly sure is the same thing, sans state management. My only concern is that the test we have that covers this change, and the resultant updated test both feel somewhat artificial to me, and Im not entirely confident that the tests prove that the code functions according to the intended use case. But I feel like the only better way to test what we're doing there is down the path of #3031379: Add a new test type to do real update testing

I'm going to re-set this to RTBC. For new code like this, I'd rather not let perfect to be the enemy of the good.

alexpott’s picture

Here's some slight improvements with a bit less complexity. And our ScaffoldFileCollection is now the iterable it's name hints it should be. yay.

Leaving at rtbc because of #209 - the changes are small.

Mixologic’s picture

I've updated the pseudo subtree split here: https://github.com/drupal/drupal-scaffold

Which handily gives us a nice interdiff of everything we've fixed since #189: https://github.com/drupal/drupal-scaffold/commit/74ffdf4ad5c4312993107ce...

Mile23’s picture

"I'd rather not let perfect to be the enemy of the good."

+1

We'll be iterating the heck out of this and #3057094: Add Composer vendor/ hardening plugin to core to get to #2982680: Add composer-ready project templates to Drupal core.

jibran’s picture

Should we mention these folks in commit message?

greg.1.anderson’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.83 KB
219.52 KB

I don't want the perfect to be the enemy of the good either, but I think that the unset would be confusing here, as it would make the appended scaffold file disappear from the project output for its original package.

Instead, I put the SkipOp in place invariantly, regardless of whether the original location is being overridden by a completely new operation or a conjoinable operation.

greg.1.anderson’s picture

Also, +1 on #213

webchick credited Xano.

webchick credited derhasi.

webchick credited hctom.

webchick credited pingers.

webchick’s picture

Added everyone from https://github.com/drupal-composer/drupal-scaffold/graphs/contributors to the issue credit, except for ojacquet who I couldn't find.

greg.1.anderson’s picture

Actually, #214 is more a case of the imperfect being the enemy of the good, as I forgot to update the tests.

Here is a corrected patch with interdiff.

webchick credited Jax.

webchick’s picture

Solved the @ojacquet mystery! (Thanks, @borisson_!)

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

RTBC-ing my own thing because yolo and the change is very small. Anyone who disagrees can "Needs Work" me.

Thanks for adding collaborators to the issue credit, @webchick.

larowlan’s picture

Adding review credits

  • larowlan committed 3bb50b2 on 8.8.x
    Issue #2982684 by greg.1.anderson, Mile23, Mixologic, webflo, alexpott,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

🚀 we have lift off

published change record

Committed 3bb50b2 and pushed to 8.8.x. Thanks!

Mile23’s picture

vijaycs85’s picture

🎉🎉🎉

kim.pepper’s picture

👏👏👏

fazni’s picture

Thanks +1

ultimike’s picture

Woot!!!

mondrake’s picture

Status: Fixed » Needs review

Is it possible that this broke testing in HEAD for PHP 7.3.dev? I see all commit tests https://www.drupal.org/pift-ci-job/1345923 failing since then.

EDIT - #2978261: \Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait::assertCacheContexts() is unhelpful after conversion to phpunit seems related.

greg.1.anderson’s picture

I don't think that the failure is related to this patch. All of the failures are reported on the same line in an unrelated file. I looked at the source and was uncertain why the error, Undefined variable: match, is happening, since $match is invariantly assigned a few lines up.

greg.1.anderson’s picture

Status: Needs review » Fixed

Whatever was wrong with the 7.3-dev build has been cleared up; tests are passing again. Not related to this patch.

jhodgdon’s picture

FYI -- I had a random test fail in ManageGitIgnoreTest yesterday. Hit retest and it passed (on the same patch). I created an issue for that:
#3067768: Random test fail in ManageGitIgnoreTest

This was not PHP 7.3, it was PHP 7.1 & MySQL 5.7. There may be a problem...

FeyP’s picture

Unfortunately I just experienced what I think is another random test fail related to this issue. I filed #3070853: Race condition during composer cache clear in functional composer scaffolding plugin tests for that. I'd appreciate it, if any of you could look into it.

Mixologic’s picture

Status: Fixed » Closed (fixed)

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

Krzysztof Domański’s picture

xjm’s picture

Issue tags: +8.8.0 highlights

This probably merits a mention in the release highlgihts?

Mixologic’s picture

+1 to #245