Problem/Motivation

Permissions issues with scaffold files, which are made unmodifiable by Drupal's permissions hardening code, is the most commonly reported problem with the Core Scaffolding Plugin. Presently, every scaffold operation rewrites every scaffold file, regardless of whether it has changed or not. This can cause spurious failures, e.g. if an unmodified scaffold file is not writable. The scaffold files protected by permissions hardening are usually not modified by users, so avoiding rewrites of these files will reduce occurrences of this problem substantially, with most users only affected when an example file in sites/default is updated by a Drupal upgrade.

The re-scaffolding also produces a lot of extraneous output to the console, as the Scaffold Plugin reports on every file that it writes. In contrast, other Composer commands, e.g. `composer install`, only print output when they are actually doing something, and remain silent in other cases, e.g. if a package has already been downloaded.

Proposed resolution

Do not scaffold a file unless the on-disk version differs from what would be written. Show the scaffold information only for files that are actually written.

Remaining tasks

Commit

User interface changes

Users no longer see excessive information about scaffold files being re-copied at the end of every composer command (install, update, etc).

API changes

None. Only internal to composer plugins (which aren't API). See #3103918: [policy + docs] Decide on backwards compatibility policy for Composer plugins in Drupal 8.

Data model changes

None.

https://www.drupal.org/project/drupal/issues/3091285

Release notes snippet

The output of the `composer scaffold` command is now much less verbose. Scaffold operations are only displayed on the first run, or when a file is changed (e.g. rewritten). This change also greatly reduces the number of times that scaffold files are rewritten, which avoids the common problem with permissions hardening that causes scaffolding to fail after the Drupal installer runs.

CommentFileSizeAuthor
#51 3103090-8.9.x-51.patch29.91 KBjungle
#51 raw-interdiff-49-51.txt1.73 KBjungle
#49 3103090-9.x-49.patch29.64 KBgreg.1.anderson
#49 48-to-49-interdiff.txt1.1 KBgreg.1.anderson
#48 3103090-9.0.x-48.patch29.64 KBgreg.1.anderson
#48 45-to-48-interdiff.txt4.36 KBgreg.1.anderson
#45 3103090-42.patch28.21 KBalexpott
#45 3103090-9.x-45.patch28.28 KBalexpott
#42 3103090-42.patch28.21 KBravi.shankar
#36 3103090-36.patch28.19 KBgreg.1.anderson
#36 26-to-36-interdiff.txt3.52 KBgreg.1.anderson
#26 3103090-26.patch27.88 KBgreg.1.anderson
#26 23-to-26-interdiff.txt1.75 KBgreg.1.anderson
#23 3103090-23.patch27.88 KBgreg.1.anderson
#23 3103090-failing.patch3.35 KBgreg.1.anderson
#13 3103090-13.patch24.54 KBgreg.1.anderson
#13 9-to-13-interdiff.txt1.58 KBgreg.1.anderson
#9 3103090-9.patch24.53 KBgreg.1.anderson
#9 5-to-9-interdiff.txt2.55 KBgreg.1.anderson
#5 3103090-5.patch23.08 KBgreg.1.anderson
#5 2-to-5-interdiff.txt17.65 KBgreg.1.anderson
#2 3103090-2.patch25.6 KBgreg.1.anderson
#2 3092563-102-to-2.interdiff.txt29.45 KBgreg.1.anderson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greg.1.anderson created an issue. See original summary.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Active » Needs review
FileSize
3092563-102-to-2.interdiff.txt29.45 KB
3103090-2.patch25.6 KB

Here is a patch. This is a reduced version of the work from #3092563: Avoid overwriting .htaccess changes during scaffolding > security problem, so I have included an interdiff for the benefit of anyone coming here from that issue.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Needs work

Cool. So we want to filter out Ops for files that we don't need to update by comparing their destination content before we do a file operation. That way we can avoid (some) errors for hardened files and the user doesn't see a wall of output per scaffolding run. We get that from ScaffoldFileCollection::checkUnchanged().

As for a review: Looks good. Mostly some minor stuff.

  1. +++ b/composer/Plugin/Scaffold/Operations/AbstractOperation.php
@@ -9,6 +9,26 @@
+  protected $contents;

    Needs a docblock. 

  2. +++ b/composer/Plugin/Scaffold/Operations/AppendOp.php
index 83b2441271..0000000000
--- a/composer/Plugin/Scaffold/Operations/ConjunctionOp.php

--- a/composer/Plugin/Scaffold/Operations/ConjunctionOp.php
+++ /dev/null

    Farewell, ConjunctionOp. However, there are still references to it in docblocks in ScaffoldFileCollection, maybe other places too. 

  3. +++ b/composer/Plugin/Scaffold/Operations/OperationInterface.php
@@ -11,6 +11,14 @@
+   * Return the exact data that will be written to the scaffold files.
+   *
+   * @return string
+   *   Contents of the scaffold file.

    Contents of the destination file, not contents of the scaffold file(s).

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

+1 to improving the UX here.

While we're nitpicking, a few more nits / questions / concerns:

  1. +++ b/composer/Plugin/Scaffold/Handler.php
@@ -161,8 +161,14 @@ public function scaffold() {
+    // write. We can remove these from consiteration, as rewritng would be a

    s/consiteration/consideration/
    s/rewritng/rewriting/ 

  2. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +72,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+   * Filter out all item in the parameter list from our collection.

    s/item/items/ 

  3. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +72,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+      $hasAtLeastOneWithContent = false;

    This seems to be a local variable for this method only. Isn't our convention to use $has_at_least_one_with_content for these, instead? 

  4. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +72,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
   public function getIterator() {
-    return new \RecursiveArrayIterator($this->scaffoldFilesByProject, \RecursiveArrayIterator::CHILD_ARRAYS_ONLY);
+    return new \ArrayIterator($this->scaffoldFilesByProject);
   }

    This seems like an important API change. Without looking more closely, not sure why we're changing the nature of the iterator for this operation and why this is safe/okay. Maybe some code comments would help? 

  5. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -103,4 +129,36 @@ public static function process(ScaffoldFileCollection $collection, IOInterface $
+   * Note that there are two reasons a file may have changed:
+   *   - The user modified it after it was scaffolded.
+   *   - The package the file came to was updated, and the file is different in
+   *     the new version.
+   *
+   * With the current scaffold code, we cannot tell the difference between the
+   * two.

    Excellent comment, thanks! Do we want a @todo / @see for "we cannot tell the difference..." part? 

  6. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -103,4 +129,36 @@ public static function process(ScaffoldFileCollection $collection, IOInterface $
+          if ($desiredContents == $actualContents) {

    can this be ===? 

  7. +++ b/core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ComposerHookTest.php
@@ -76,46 +68,75 @@ protected function tearDown() {
   /**
-   * Test to see if scaffold operation runs at the correct times.
+   * Test to see if scaffold operation runs after 'composer require'.
    */
-  public function testComposerHooks() {
-    $this->fixturesDir = $this->fixtures->tmpDir($this->getName());
-    $replacements = ['SYMLINK' => 'false', 'PROJECT_ROOT' => $this->projectRoot];
-    $this->fixtures->cloneFixtureProjects($this->fixturesDir, $replacements);
+  public function testRequireHooks() {

    Seems good but not obvious why this is in scope with the current issue... 

  8. +++ b/core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ComposerHookTest.php
@@ -76,46 +68,75 @@ protected function tearDown() {
-    $this->assertScaffoldedFile($sut . '/sites/default/default.settings.php', FALSE, 'scaffolded from the scaffold-override-fixture');
+    $this->assertScaffoldedFile($sut . '/sites/default/default.settings.php', FALSE, 'Test version of default.settings.php from drupal/core');
+
     // Delete the same test scaffold file again, then run
     // 'composer drupal:scaffold' and see if the scaffold file is
     // re-scaffolded.
     @unlink($sut . '/sites/default/default.settings.php');
     $this->assertFileNotExists($sut . '/sites/default/default.settings.php');
     $this->mustExec("composer install --no-ansi", $sut);
-    $this->assertScaffoldedFile($sut . '/sites/default/default.settings.php', FALSE, 'scaffolded from the scaffold-override-fixture');
+    $this->assertScaffoldedFile($sut . '/sites/default/default.settings.php', FALSE, 'Test version of default.settings.php from drupal/core');
+

    More good test cleanup that should maybe be a follow-up to make this smaller / easier to review?

Thanks!
-Derek

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Issue summary: View changes
FileSize
2-to-5-interdiff.txt17.65 KB
3103090-5.patch23.08 KB

#3.1: Added docblock.

#3.2: The code that combined operations into conjunction operations is still present and only slightly changed by this patch; that's why references to 'conjunction' still existed. I renamed these methods for more consistency and clarity with the new algorithm.

#3.3: Improved docblock comment.

#4.1, #4.2: Fixed typos.

#4.3: Fixed Drupal coding convention.

#4.4: This question also came up in #3092563-51. My answer from last time:

The existing iterator method was unused. I needed it for this refactor, but the recursive array iterator was not behaving as expected. I switched to using an array iterator instead for simplicity.

ArrayIterator seems like a good choice for a method called getIterator. I'm not sure why the old code was using a recursive array iterator, but seeing as the method was unused, perhaps it does not matter.

#4.5: Added a reference to the issue that provides more code to allow us to distinguish between these two cases.

#4.6: Used `===`; probably does not matter, as contents should never be any type other than a string.

#4.7, #4.8: These changes are for another patch. They are simplifications to the tests, not necessary here, so I backed them out.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs work » Needs review

Back to 'needs review' to run tests.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

I really wish we could get over the use of the term 'nitpick.' For instance:

that's why references to 'conjunction' still existed.

I mean this, which is still there:

  public function __construct(array $file_mappings, Interpolator $location_replacements) {
    // Collection of all destination paths to be scaffolded. Used to determine
    // when two project scaffold the same file and we have to skip or use a
    // ConjunctionOp.

[..]
        // If there was already a scaffolding operation happening at this path,
        // and the new operation is Conjoinable, then use a ConjunctionOp to
        // join together both operations. This will cause both operations to
        // run, one after the other. At the moment, only AppendOp is
        // conjoinable; all other operations simply replace anything at the same
        // path.

It's confusing to read if there's no such thing as a ConjoinOp. The curse of good inline documentation is that you have to keep it updated...

Re: #4.4: I'd argue that there's no such thing as an API change in a Composer plugin. You can't subclass anything from a plugin package without also inheriting its events and behaviors. You pretty much have to fork a Composer plugin if you want to change it.

In fact, we might consider marking our plugins @internal just to be explicit. That would be a follow-up.

Also #4.8: Those tests are from before build tests were available. We might consider figuring out if we should convert them to build tests for a little less complexity, or if they'd get more complex in the process...

Generally +1 on everything else.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

Darn it, I thought I searched and found all of the conjunction / conjoin references. I'll go back and clobber the rest.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
FileSize
5-to-9-interdiff.txt2.55 KB
3103090-9.patch24.53 KB

that's why references to 'conjunction' still existed.

Also, that was an explanation, not an excuse. :) The feedback was valid and appropriate. These should all be gone now.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Ah, mixup on my part. I meant to direct the 'nitpick' gripe more towards @dww, which I really shouldn't have done anyway, but now I messed it up and here we are.

+1 on docs change, and thanks.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Re: "nitpicking" in #4: Sorry that came across as a criticism of the review in #3! Totally unintentional. What I really meant is:

"While this needs work and will be re-rolled, here's a mostly nitpick-ful review for other minor things to fix about this otherwise nice-looking patch."

@Mile23 sorry that you took offense. Definitely agree that the points in #3 (while mostly cosmetic) are totally valid.

@greg.1.anderson thanks for re-rolling to address all the feedback!

One very minor remaining thing I see:

+++ b/composer/Plugin/Scaffold/Operations/OperationInterface.php
@@ -11,6 +11,14 @@
+   * Return the exact data that will be written to the scaffold files.

+++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +69,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+   * Filter out all items in the parameter list from our collection.

@@ -103,4 +126,36 @@ public static function process(ScaffoldFileCollection $collection, IOInterface $
+   * Return the list of files that have not changed since they were scaffolded.

UberNit: Core's standards for doc blocks are action verbs that end in 's'. E.g. s/Return/Returns/ and s/Filter/Filters/.

Otherwise, I'd RTBC, except I can't figure out how to actually test this patch. ;) If I start from a git checkout of 8.9.x branch, the patch applies, but the composer.json isn't setup as a real composer-based site, scaffolding doesn't happen at all, and there's nothing to see/verify about this change.

If I start from
composer create-project drupal/recommended-project 8_8_composer --no-interaction
I get a test site that uses the scaffolding stuff, see the problem, but if I try to install this patch in the site, it doesn't apply at all. There is no composer subdir of either the 8_8_composer root dir, web nor web/core. It seems the relevant composer scaffolding stuff is now in vendor/drupal/core-composer-scaffold but with a different structure than what this patch expects.

How does one actually test this on a working composer-based site? ;)

Thanks!
-Derek

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

@dww The easiest way to set up to do ad-hoc testing on this site is to run the functional tests like this:

SCAFFOLD_FIXTURE_DIR=$HOME/tmp/scaffold-fixtures ./vendor/bin/phpunit -c core --group Scaffold

Then you can cd into the fixtures that are created inside $HOME/tmp/scaffold-fixtures, and run composer scaffold et. al. on some of the fixtures you find there, like drupal-drupal or drupal-composer-drupal-project (n.b. these fixtures are not actually very similar to their namesakes).

If you want to test this on an actual site you have set up locally, copy the path repository directives from those fixtures into your site's composer.json file.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
FileSize
9-to-13-interdiff.txt1.58 KB
3103090-13.patch24.54 KB

Here's fix for the comment summary conventions.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Re: #12: Perfect, thanks! That works great.

Before

% cd /.../tmp/scaffold-fixtures/drupal-drupal
% composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files
Scaffolding files for fixtures/drupal-assets-fixture:
  - Copy [web-root]/.csslintrc from assets/.csslintrc
  - Copy [web-root]/.editorconfig from assets/.editorconfig
  - Copy [web-root]/.eslintignore from assets/.eslintignore
  - Copy [web-root]/.eslintrc.json from assets/.eslintrc.json
  - Copy [web-root]/.gitattributes from assets/.gitattributes
  - Copy [web-root]/.ht.router.php from assets/.ht.router.php
  - Skip [web-root]/.htaccess: overridden in fixtures/drupal-drupal
  - Copy [web-root]/sites/default/default.services.yml from assets/default.services.yml
  - Skip [web-root]/sites/default/default.settings.php: overridden in fixtures/scaffold-override-fixture
  - Copy [web-root]/sites/example.settings.local.php from assets/example.settings.local.php
  - Copy [web-root]/sites/example.sites.php from assets/example.sites.php
  - Copy [web-root]/index.php from assets/index.php
  - Skip [web-root]/robots.txt: overridden in fixtures/drupal-drupal
  - Copy [web-root]/update.php from assets/update.php
  - Copy [web-root]/web.config from assets/web.config
Scaffolding files for fixtures/scaffold-override-fixture:
  - Copy [web-root]/sites/default/default.settings.php from assets/override-settings.php
Scaffolding files for fixtures/drupal-drupal:
  - Skip [web-root]/.htaccess: disabled
  - Copy [web-root]/robots.txt from assets/robots-default.txt
% rm index.php
% composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files
Scaffolding files for fixtures/drupal-assets-fixture:
  - Copy [web-root]/.csslintrc from assets/.csslintrc
  - Copy [web-root]/.editorconfig from assets/.editorconfig
  - Copy [web-root]/.eslintignore from assets/.eslintignore
  - Copy [web-root]/.eslintrc.json from assets/.eslintrc.json
  - Copy [web-root]/.gitattributes from assets/.gitattributes
  - Copy [web-root]/.ht.router.php from assets/.ht.router.php
  - Skip [web-root]/.htaccess: overridden in fixtures/drupal-drupal
  - Copy [web-root]/sites/default/default.services.yml from assets/default.services.yml
  - Skip [web-root]/sites/default/default.settings.php: overridden in fixtures/scaffold-override-fixture
  - Copy [web-root]/sites/example.settings.local.php from assets/example.settings.local.php
  - Copy [web-root]/sites/example.sites.php from assets/example.sites.php
  - Copy [web-root]/index.php from assets/index.php
  - Skip [web-root]/robots.txt: overridden in fixtures/drupal-drupal
  - Copy [web-root]/update.php from assets/update.php
  - Copy [web-root]/web.config from assets/web.config
Scaffolding files for fixtures/scaffold-override-fixture:
  - Copy [web-root]/sites/default/default.settings.php from assets/override-settings.php
Scaffolding files for fixtures/drupal-drupal:
  - Skip [web-root]/.htaccess: disabled
  - Copy [web-root]/robots.txt from assets/robots-default.txt

After

% composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files
% rm index.php
% composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files
Scaffolding files for fixtures/drupal-assets-fixture:
  - Skip [web-root]/.htaccess: overridden in fixtures/drupal-drupal
  - Copy [web-root]/index.php from assets/index.php

Any ideas why with the patch applied, after removing only index.php, we're getting a notice about .htaccess?

  - Skip [web-root]/.htaccess: overridden in fixtures/drupal-drupal

Otherwise, this seems great and RTBC. I have no further concerns on the patch / docs. However, I'll hold off on the status change until confirmation around the .htaccess thing.

Thanks again,
-Derek

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

The .htaccess thing is by design. The message you are seeing is letting you know that .htaccess was NOT written, and the reason why it was not written.

Prior to this issue, all messages were printed all the time.

After this issue, we only print messages when the operation changes a file. But what about those notification messages, like the .htaccess override? They never change files. These messages, then, are printed any time any other file change happens as the result of any other operation from the same package changing the contents of a file. This means that the notifications are printed on the first run, when all messages are printed, and they are not printed on a run where no files are changed (so no output at all is produced).

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Okay, basically makes sense. However, in that case, why aren't we seeing the notice about other skipped files? E.g:

  - Skip [web-root]/robots.txt: overridden in fixtures/drupal-drupal
greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs review » Needs work

Probably because I still have one more bug to track down.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs work » Needs review

#16 actually isn't a bug.

The robots.txt file still exists on disk, so the Scaffold plugin knows that it wasn't modified, and it filters it out. In the case of the .htaccess file, though, there is no file on disk. Therefore, the Scaffold plugin figures it had better write it back to disk again, but all it has is the "skip" message. The important difference here is that .htaccess is set to 'false', whereas the robots.txt file is appended, so there are contents for the plugin to compare against.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

We could improve this behavior if the Scaffold plugin recorded something about the state of the source file at the time it was written (e.g. its hash), but in this issue we do not support that.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs tests

@greg.1.anderson: Thanks for investigating and clarifying! All makes sense. I was about to RTBC, but I realized we should probably have a test that shows we don't try to scaffold or print out anything about an un-touched file. Sorry for not noticing this and mentioning it earlier. I looked at all the test changes in the patch and don't see anything like this. Would you be willing to add something?

Perhaps also attach a test-only patch that shows the test failure without the fixes applied, and the full patch with the fixes and test that should be green?

Thanks/sorry!
-Derek

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Pasting Slack exchange here to clarify:

greg.1.anderson 10 minutes ago
@dww Sure, that sounds reasonable. To be clear, the only test you want is one that confirms nothing is printed on the second run when running composer scaffold (or equiv) twice, yes?

dww 9 minutes ago
Yeah, basically.

dww 9 minutes ago
Obv, that command always prints something. ;)

dww 7 minutes ago
Ideally, something more or less like the commands / output I ran as before/after in comment 14.... Do initial scaffolding, verify it worked. Run scaffold again, expect to see nothing. Remove a file. Run scaffold again, expect to see only the file I removed as re-written.

dww 6 minutes ago
I assume "Do initial scaffolding, verify it worked." already exists as a test somewhere... so this would be more actions + assertions at the end of the same test method...

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

Ironically, #20 is related to #4.7 and #4.8. I had tests like this in the issue that I forked this from, but backed out too much when I ported over to here, and then removed the rest per #4. No worries, though; the suggested test is valuable. I should have noticed it was missing when I looked at #4. I'll add it back in.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs work » Needs review
FileSize
3103090-failing.patch3.35 KB
3103090-23.patch27.88 KB

Here are the moar tests, along with a failing test.

Oh, the failing test is also by coincidence the interdiff from the last patch.

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

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Needs work

@greg.1.anderson: Fantastic! That's (almost) ideal.

I was about to RTBC (again!), but then I looked at the test results for the green patch. Feels cruel to do this, but NW for the CS warnings:

5 coding standards messages
✗ 5 more than branch result

/var/lib/drupalci/workspace/jenkins-drupal_patches-23927/source/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php ✗ 3 more
line 82 TRUE, FALSE and NULL must be uppercase; expected "FALSE" but found "false"
87 Expected 1 space after ELSEIF keyword; 0 found
90 TRUE, FALSE and NULL must be uppercase; expected "TRUE" but found "true"
/var/lib/drupalci/workspace/jenkins-drupal_patches-23927/source/core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ComposerHookTest.php ✗ 2 more
153 Expected 1 blank line after function; 2 found
156 The closing brace for the class must have an empty line before it

I'd just fix these myself, but then I couldn't officially RTBC.

Sorry!
-Derek

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs work » Needs review
FileSize
23-to-26-interdiff.txt1.75 KB
3103090-26.patch27.88 KB

np, thanks for the review. Here are the cs fixes.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Fantastic, thanks! 🙏
No moar complaints. ;) Removing tag. 👍
Ready To Be Committed (the original meaning of "RTBC") 🎉

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Title: Avoid printing scaffold file information over and over » Avoid re-scaffolding unchanged files (and printing scaffold file information over and over)
Issue summary: View changes

Minor cleanup:
- More descriptive title of what this patch actually does.
- Hide some stale files.
- Minor summary updates / clarifications.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Just a +1 that the test in #23 does in fact check that we don't re-scaffold files that are already present.

Then in #3092563: Avoid overwriting .htaccess changes during scaffolding > security problem we can add more complex UX type changes as to how to handle files that the user has altered but didn't set up their own scaffolding for.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review
  1. The OP says

    None. Only internal to composer plugins (which aren't API).

    for API changes - can you point me to that policy - thanks.

  2. +++ b/composer/Plugin/Scaffold/Operations/AppendOp.php
@@ -91,33 +118,20 @@ public function process(ScaffoldFilePath $destination, IOInterface $io, Scaffold
-    if (!empty(trim($prepend_contents)) || !empty(trim($append_contents))) {

    we seem to have lost the trim in the new logic - does that matter? 

  3. +++ b/composer/Plugin/Scaffold/Operations/AppendOp.php
@@ -126,16 +140,17 @@ public function process(ScaffoldFilePath $destination, IOInterface $io, Scaffold
+  public function scaffoldOverExistingTarget(OperationInterface $existing_target) {
+    $this->originalContents = $existing_target->contents();
+    return $this;

    is there something to prevent the prepend/append happening every time?

    e.g. the file is originally made up of original contents, but then on the first pass we get prepend/original/append - what mechanism is there to prevent us getting prepend/prepend/original/append/append on subsequent attempts? 

  4. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -34,8 +34,9 @@ class ScaffoldFileCollection implements \IteratorAggregate {
+    // when two project scaffold the same file and we have to either replace or

    🧐 nit: projects 

  5. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +69,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+  public function filterFiles($files_to_filter) {

    🧐 nit: we can typehint $files_to_filter as an array 

  6. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +69,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+        if (in_array($destination_rel_path, $files_to_filter)) {

    🧐nit: should use the third argument on in_array because strings 

  7. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +69,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+          unset($this->scaffoldFilesByProject[$project_name][$destination_rel_path]);

    🧐 nit: we can continue here and then make the elseif an if 

  8. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +69,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+            $has_at_least_one_with_content = TRUE;

    🧐 nit: I think we can break here or continue 2 - we've found one file with content for this project, so we can plough on?

    If we do that, we don't even need the local $has_at_least_one_with_content variable I think 🤔 

  9. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -72,18 +69,44 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+    return new \ArrayIterator($this->scaffoldFilesByProject);

@@ -92,9 +115,9 @@ public function getIterator() {
+    foreach ($this as $project_name => $scaffold_files) {

    😎 

  10. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -103,4 +126,36 @@ public static function process(ScaffoldFileCollection $collection, IOInterface $
+          $desiredContents = $scaffold_file->op()->contents();
+          $actualContents = file_get_contents($path);
+
+          if ($desiredContents === $actualContents) {
+            $results[] = $scaffold_file->destination()->relativePath();

    I think this answers my question above about 'what safeguards are there' to prevent subsequent prepending appending? 

  11. +++ b/core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ComposerHookTest.php
@@ -136,4 +125,31 @@ public function testComposerHooks() {
+    // First test: run composer install. This is the same as composer update
...
+    $stdout = $this->mustExec("composer install --no-ansi", $sut);
...
+    $stdout = $this->mustExec("composer scaffold --no-ansi", $sut);
...
+    $stdout = $this->mustExec("composer scaffold --no-ansi", $sut);

    🤔 nit: I think we have one too many mustExecs here?

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs review » Needs work

1. Opened #3103918: [policy + docs] Decide on backwards compatibility policy for Composer plugins in Drupal 8 to track this. Note that "compatibility" for a Composer plugin has a different definition than for other sorts of APIs. In some ways a Composer plugin is more flexible to being changed without impacting any users. Other kinds of changes, though, can cause composer update to crash, as outlined in the referenced issues. These sorts of changes cannot simply be postponed until the next stable release, as they would also cause a composer update crash when moving, say, from 8.9.x to 9.0.x.

I will make a test to guard against these sorts of changes so that we do not accidentally get ourselves into a position where it's hard for users to upgrade Drupal versions only due to Composer plugins.

2. The old logic used 'trim' when determining whether the append / prepend contents were empty. The new logic does not care whether the append / prepend contents are empty; it is now concerned with the final file contents. We could still trim the append / prepend file contents before adding it to the original scaffold file contents. That didn't seem necessary to me, but I'd be fine with doing that if anyone thought it was preferable. The current algorithm does not trim the contents before appending / prepending, though, only for comparing.

3. The answer in 10. is correct.

I'll update the patch to address the other review points. I prefer to avoid `break` and `continue` most of the time; I will either find another way to avoid the elseif, or just go ahead and address as suggested.

Setting to "needs work" for 1.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs work » Postponed

Postponed on #3104922: Guard against changes to Scaffold Plugin that might cause upgrade problems .

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

For what it's worth, I merged #3104922: Guard against changes to Scaffold Plugin that might cause upgrade problems in here, and with slight modifications, the tests passed. (Slight modifications were needed because the tests as written in the other patch used output in stdout to determine that the scaffold plugin was working, but this patch optimized away that output because the scaffold operation wasn't necessary.)

I split out #3104922 because I feel that the Scaffold plugin shipped in 8.8.0 and 8.8.1 is a little dangerous, and I'd like to see that patch get into 8.8.2, so that we have the safer version available as soon as possible.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Postponed » Needs work

Un-postponed here now that #3104922: Guard against changes to Scaffold Plugin that might cause upgrade problems is in. Will re-roll shortly.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Category: Task » Bug report
Issue summary: View changes

I updated the Issue Summary to more correctly classify this issue as a bug, as the primary benefit of this issue is avoiding scaffold failures after permissions hardening.

30.1. I updated the issue summary in #3103918: [policy + docs] Decide on backwards compatibility policy for Composer plugins in Drupal 8 to contain proposed text clarifying this policy. #3104922: Guard against changes to Scaffold Plugin that might cause upgrade problems has been committed, providing protection against changes to the Scaffold plugin which might cause update problems.

I still need to make minor changes to address #30 and re-roll.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs work » Needs review
FileSize
26-to-36-interdiff.txt3.52 KB
3103090-36.patch28.19 KB

#30.1 - #30.3: Addressed above.

#30.4 - #30.6: Fixed minor defects as identified.

#30.7 & #30.8: Refactored for better code hygiene.

#30.11: Removed extraneous scaffold operation.

Could someone be so kind as to review here and set to POSTPONED (on #3104922: Guard against changes to Scaffold Plugin that might cause upgrade problems ) if it's RTBC, or set it to NEEDS WORK if it's not?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

#3104922: Guard against changes to Scaffold Plugin that might cause upgrade problems is in for 8.9x and 9.0.x, with a possible 8.8.x backport.

LGTM.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: 3103090-36.patch, failed testing. View results

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Reviewed & tested by the community

It seems unrelated test failures, so back to RTBC as per comment #37.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

We need a roll here.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Assigned: Unassigned » ravi.shankar
ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Assigned: ravi.shankar » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3103090-42.patch28.21 KB

Here I have added re-roll of patch #36.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC based on #37. Thanks, @ravi.shankar

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
3103090-9.x-45.patch28.28 KB
3103090-42.patch28.21 KB

9.x and 8.x have diverged for COmposerHookTest and the new test is using things deprecated in Drupal 9. So here's a 9.x patch - plus the old 8.x patch for good measure.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work

This is looking pretty good. Whilst I was the originator of the conjoining approach I was always a little unsure of the approach - I felt it was the best we had at the time. This change (that removes it) I think is a move in a really good direction and the system is clearer for it. Nice work.

  1. +++ b/composer/Plugin/Scaffold/Operations/AbstractOperation.php
@@ -11,17 +11,42 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function contents() {

    If we make this final then we enforce the relationship between this and generateContents() which seems a good idea to me. It feels like the point here is to not generate contents more than once AND which also sort of enforces the calling contents twice on the smae operation cannot return something different - which feels an important thing. 

  2. +++ b/composer/Plugin/Scaffold/Operations/AbstractOperation.php
@@ -11,17 +11,42 @@
+   * Load the scaffold contents or otherwise generate what is needed.
+   *
+   * @return string
+   *   The contents of the scaffold file to be cached.
+   */
+  abstract protected function generateContents();

    I think the to be cached is not necessary - it feels like an implementation detail.

    I think the purpose of the this method is get the contents of the file with the operation applied. 

  3. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -74,18 +71,57 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+   * @param array $files_to_filter

    Can we typehint on SomeObject[] 

  4. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -74,18 +71,57 @@ public function __construct(array $file_mappings, Interpolator $location_replace
+   * @param array $scaffold_files

    Can we typehint to ScaffoldFileInfo[]? 

  5. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
@@ -105,4 +141,36 @@ public static function process(ScaffoldFileCollection $collection, IOInterface $
+        if (file_exists($path)) {
+          $desiredContents = $scaffold_file->op()->contents();
+          $actualContents = file_get_contents($path);
+
+          if ($desiredContents === $actualContents) {
+            $results[] = $scaffold_file->destination()->relativePath();
+          }
+        }

    Does this hint that we should have a $scaffold_file->hasChanges() method that does something like

    $path = $this->destination()->fullPath();
if (!file_exists($path)) {
  return TRUE;
}
return $this->op()->contents() !== file_get_contents($path)
greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

I agree with all of the suggestions in #46. I'll roll a new patch as soon as I get a chance, if someone else doesn't pick it up first.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs work » Needs review
FileSize
45-to-48-interdiff.txt4.36 KB
3103090-9.0.x-48.patch29.64 KB

Here are fixes for #46 for the 9.0.x branch. Don't quite have time to do a quick back-port to 8.9.x at the moment.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
FileSize
48-to-49-interdiff.txt1.1 KB
3103090-9.x-49.patch29.64 KB

Coding standards.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Assigned: Unassigned » jungle

On rerolling a patch for 8.9.x

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Assigned: jungle » Unassigned
FileSize
raw-interdiff-49-51.txt1.73 KB
3103090-8.9.x-51.patch29.91 KB

assertStringContainsString() and assertStringNotContainsString() are not available in 8.x, but with the forwards-compatibility shim introduced in #3126797: [D8 only] Add forwards-compatibility shim for assertString(Not)ContainsString()replacements in phpunit 6&7, it's good to go.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Needs review » Reviewed & tested by the community

All my feedback from #46 has been addressed thanks @greg.1.anderson.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed c1548fce81 to 9.1.x and 4b53364f22 to 9.0.x. Thanks!

Committed 39ec77f and pushed to 8.9.x. Thanks!

I backported this to 8.9.x (would love to put it in 8.8.x too but let's not) - because this addresses a very annoying bug in scaffold and allows most users to also fix #3091285: Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable..

  • alexpott committed c1548fc on 9.1.x 
    Issue #3103090 by greg.1.anderson, alexpott, jungle, ravi.shankar, dww,...

  • alexpott committed 4b53364 on 9.0.x 
    Issue #3103090 by greg.1.anderson, alexpott, jungle, ravi.shankar, dww,...

  • alexpott committed 39ec77f on 8.9.x 
    Issue #3103090 by greg.1.anderson, alexpott, jungle, ravi.shankar, dww,...
jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Related issues: +#3133162: Replace the start verb Test with Tests in method comments of tests

@alexpott, thanks for committing!

+++ b/core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ComposerHookTest.php
@@ -136,4 +127,29 @@ public function testComposerHooks() {
+   * Test to see if scaffold messages are omitted when running scaffold twice.
+   */
+  public function testScaffoldMessagesDoNotPrintTwice() {

Test ... should be Tests ..., but the patches were committed, and found there are more occurences, so filed #3133162: Replace the start verb Test with Tests in method comments of tests

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

Yay! Thanks for committing! This should help the experience of scaffolding a whole bunch.

Status: Fixed » Closed (fixed)

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