Problem/Motivation

There are scenarios where a failure is reported when perform various composer commands such as
composer require

  [RuntimeException]                                                                                               
  Could not delete /path/web/sites/default/default.services.yml: 
...
  Installation failed, reverting ./composer.json to its original content.

#3103090: Avoid re-scaffolding unchanged files (and printing scaffold file information over and over) has been merged. This should take care of the symptom historically described in this issue in most instances, as least when using drupal/{recommended,legacy}-project without any additional scaffold files. In that scenario, the only time the failure reported in this issue should be seen is:

  1. The site builder modifies sites/default/default.services.yml or sites/default/default.settings.php. (Should be rare, as these files should be copied before they are modified.)
  2. The site builder removes sites/default/default.services.yml or sites/default/default.settings.php.
  3. Whenever Drupal core changes either file:
    $ git log --pretty=format:"%ad%x09%<(25,trunc)%s" -n 10 origin/10.1.x -- sites/default/default.*
    Wed Apr 19 11:18:37 2023 -0500  SA-CORE-2023-005 by ben..
    Tue Apr 11 14:10:23 2023 +0100  Issue #3027639 by catch..
    Sun Mar 12 20:06:51 2023 +0000  Issue #3107548 by tunic..
    Mon Mar 6 17:14:57 2023 +0000   Issue #3150614 by pfren..
    Fri Mar 3 16:08:14 2023 +0000   Revert "Issue #3150614 ..
    Fri Mar 3 11:13:53 2023 +0000   Issue #3150614 by pfren..
    Thu Feb 23 16:22:19 2023 +0000  Issue #3317265 by ressa..
    Thu Feb 23 10:20:36 2023 +0000  Issue #3198868 by dpi, ..
    Thu Feb 16 22:36:17 2023 +0000  Issue #3333281 by Musta..
    Wed Nov 30 17:35:34 2022 +0000  Issue #3032746 by mfb, ..
    

Doing either of these things will cause the next scaffold operation to fail.

Current workaround

The best workaround today as per #131.

"scripts": {
        "pre-drupal-scaffold-cmd": [
            "chmod 775 web/sites/default"
        ],
        "post-drupal-scaffold-cmd": [
            "chmod 555 web/sites/default"
        ]
    },

Proposed resolution

We could still continue with this patch in order to improve the scaffolding operation such that these workarounds are not necessary. If someone wanted to move this forward, my comments in #43 still apply.

Rather than adding so many parameters to control the behavior of the replace and append ops as proposed historically, perhaps a simpler strategy of simply trying harder would be appropriate. i.e. the scaffold plugin could always try to make a write-protected file writable first, and only fail if it does not have permission to do that.

Option A:
The scaffolding could try to `chmod` unwritable files before the operations themselves were called to attempt to write their contents.

Option B:
We could introduce the concept of an "optional" scaffold file (e.g. default.settings.php, README.txt, etc), and reduce permission denied errors to warnings for those files.

Remaining tasks

[ ] Implement option A and maybe option B

Follow-on tasks

[ ] #3092563: Avoid overwriting .htaccess changes during scaffolding > security problem

User interface changes

This should result in better and less warnings/errors during scaffolding

API changes

None

Data model changes

None, unless "option B" implemented.

Release notes snippet

Older description for historical context

If you've installed and run Drupal, the sites/default directory receives permission hardening. This can break the scaffolding plugin.

  [RuntimeException]                                                                                               
  Could not delete /path/web/sites/default/default.services.yml: 
composer require drupal/openapi drupal/openapi_ui drupal/openapi_ui_redoc drupal/schemata
    1/1:	http://repo.packagist.org/p/provider-latest$46a92f2b264b89accfae0d6579f9b6e022c87a70701dad793daf7963e2bf31df.json
    Finished: success: 1, skipped: 0, failure: 0, total: 1
Using version ^1.0@beta for drupal/openapi                
Using version ^1.0@RC for drupal/openapi_ui
Using version ^1.0@RC for drupal/openapi_ui_redoc
Using version ^1.0@beta for drupal/schemata
./composer.json has been updated
Gathering patches for root package.
Loading composer repositories with package information
Updating dependencies (including require-dev)         
Package operations: 5 installs, 0 updates, 0 removals
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Installing drupal/schemata (1.0.0-beta1): Loading from cache
  - Installing drupal/schemata_json_schema (1.0.0-beta1)
  - Installing drupal/openapi (1.0.0-beta6): Loading from cache
  - Installing drupal/openapi_ui (1.0.0-rc2): Loading from cache
  - Installing drupal/openapi_ui_redoc (1.0.0-rc2): Loading from cache
Writing lock file
Generating autoload files
Scaffolding files for drupal/core:
  - Copy [project-root]/.editorconfig from assets/scaffold/files/editorconfig
  - Copy [project-root]/.gitattributes from assets/scaffold/files/gitattributes
  - Copy [web-root]/.csslintrc from assets/scaffold/files/csslintrc
  - Copy [web-root]/.eslintignore from assets/scaffold/files/eslintignore
  - Copy [web-root]/.eslintrc.json from assets/scaffold/files/eslintrc.json
  - Copy [web-root]/.ht.router.php from assets/scaffold/files/ht.router.php
  - Copy [web-root]/.htaccess from assets/scaffold/files/htaccess
  - Copy [web-root]/example.gitignore from assets/scaffold/files/example.gitignore
  - Copy [web-root]/index.php from assets/scaffold/files/index.php
  - Copy [web-root]/INSTALL.txt from assets/scaffold/files/drupal.INSTALL.txt
  - Copy [web-root]/README.txt from assets/scaffold/files/drupal.README.txt
  - Copy [web-root]/robots.txt from assets/scaffold/files/robots.txt
  - Copy [web-root]/update.php from assets/scaffold/files/update.php
  - Copy [web-root]/web.config from assets/scaffold/files/web.config
  - Copy [web-root]/sites/README.txt from assets/scaffold/files/sites.README.txt
  - Copy [web-root]/sites/development.services.yml from assets/scaffold/files/development.services.yml
  - Copy [web-root]/sites/example.settings.local.php from assets/scaffold/files/example.settings.local.php
  - Copy [web-root]/sites/example.sites.php from assets/scaffold/files/example.sites.php

Installation failed, reverting ./composer.json to its original content.

Issue fork drupal-3091285

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mglaman created an issue. See original summary.

mglaman’s picture

Also: this causes composer.lock to update, but then compoer.json is broken.

mile23’s picture

Issue tags: +Composer initiative
mglaman’s picture

Priority: Normal » Major

I don't like to be the person who bumps priority. But this cause data loss on my end. I pulled my repo and composer.lock had my dependencies and composer.json didn't. I ran `composer update` because Composer said things were out of date. Then I lost modules.

greg.1.anderson’s picture

@mglaman: Could you clarify the situation that caused you to lose modules? It seems that the only way that composer.json would lose modules would be if the Installation failed, reverting ./composer.json to its original content step took them out. In that scenario, though, I would think that the only thing that could be removed would be modules being composer required on that run, and those would not be in the composer.lock file yet.

mglaman’s picture

#5: correct, it's only things added on the last composer require. However, the composer.lock is already updated as the dependencies are resolved.

Which is weird. I wouldn't expect Composer to revert composer.json if a plugin crashed. But it did. And I had a mismatched composer.json and composer.lock.

So when I moved from my desktop to laptop and did a git pull && composer install, it told me to run composer update because things didn't match. That's when a bunch of modules uninstalled because they were in the .lock but not .json

greg.1.anderson’s picture

Hm I tried to make a comment here prior to #5 that looks like it never got saved. Weird. Here's my first comment again.

We have to routes forward here:

1. We could just go ahead and chmod our way to success here. There's nothing we can do if someone also chown'ed the files, so we should also see if we can add a little error checking so that we at least get a good error message if a file fails to scaffold due to permissions if we go this route. My question is: should we aggressively try to chmod everything before writing scaffold files, or whitelist only the ones that Drupal makes unmodifiable? (The 'default' directory and settings.php file, etc.)

2. We could simply account for the fact that some scaffold files are unwritable, and be okay with that. If the file exists and is >0 bytes and is not writable, then skip the file with a warning message and continue. If the file does not exist or is 0 bytes and cannot be created, then fail with an error message.

Note that we can't change the behavior of what Composer does when we fail, but we can reduce the circumstances where it happens, and make the messaging better when it does.

greg.1.anderson’s picture

Discussed with @mixologic a bit, and it seems that we want to go with #2, with modifications.

Essentially, it is not up to the scaffold tool to manage your site's file permissions. In my opinion, the Drupal installer shouldn't be doing this either, but it does, so we're stuck with the situation that the user must fix their file permissions before doing any more development that modifies these unwritable files.

We should improve the way that the scaffold tool reacts when it encounters a scaffold file that it cannot write. It should instead simply report that it could not write the file, regardless of the reason or disposition of the file. Ideally there would be some conspicuous red in the message. After we whine about the inevitable ennui of the situation, we should continue as if nothing had happened.

afeijo’s picture

Status: Active » Needs review
StatusFileSize
new1.98 KB

Here it is, a patch with the changes that @greg.1.anderson instructed me to do.

mglaman’s picture

It'd also be nice if it didn't run unless drupal/core was updated. Reduces number of times this would happen

greg.1.anderson’s picture

@mglaman Reducing the number of times that the scaffold operations run is out-of-scope for this issue. We definitely want to improve the experience here, but it's non-trivial.

mglaman’s picture

Reducing the number of times that the scaffold operations run is out-of-scope for this issue.

👍I agree, just wanted to mention it.

Mixologic’s picture

+++ b/composer/Plugin/Scaffold/Operations/ReplaceOp.php
@@ -85,10 +85,11 @@ protected function copyScaffold(ScaffoldFilePath $destination, IOInterface $io)
+      $io->write($interpolator->interpolate("  - Copy <info>[dest-rel-path]</info> from <info>[src-rel-path]</info>"));

@@ -108,11 +109,11 @@ protected function symlinkScaffold(ScaffoldFilePath $destination, IOInterface $i
+      $io->write($interpolator->interpolate("  - Link <info>[dest-rel-path]</info> from <info>[src-rel-path]</info>"));

Can we add IOInterface::VERBOSE to the success messages - i.e. only be wordy if theres a problem.

Mixologic’s picture

Issue tags: +Amsterdam2019
+++ b/composer/Plugin/Scaffold/Operations/ReplaceOp.php
@@ -85,10 +85,11 @@ protected function copyScaffold(ScaffoldFilePath $destination, IOInterface $io)
+      $io->write($interpolator->interpolate("  - Copy <info>[dest-rel-path]</info> from <info>[src-rel-path]</info>"));

@@ -108,11 +109,11 @@ protected function symlinkScaffold(ScaffoldFilePath $destination, IOInterface $i
+      $io->write($interpolator->interpolate("  - Link <info>[dest-rel-path]</info> from <info>[src-rel-path]</info>"));

Can we add IOInterface::VERBOSE to the success messages - i.e. only be wordy if theres a problem.

greg.1.anderson’s picture

#14: That would be inconsistent with the current behavior of other scaffold items. Let's tackle verbosity as a follow-on. I think there's already an issue.

Mixologic’s picture

re #14 sure, that makes sense.

greg.1.anderson’s picture

Status: Needs review » Needs work
+++ b/composer/Plugin/Scaffold/Operations/ReplaceOp.php
@@ -85,10 +85,11 @@ protected function copyScaffold(ScaffoldFilePath $destination, IOInterface $io)
     return new ScaffoldResult($destination, $this->overwrite);

I feel bad that I did not catch this before; however, it is a problem that we return a ScaffoldResult even if we did not scaffold the file (because the file was unwritable). The advantage of throwing an exception on failure is that we never had to deal with unscaffoldable files in the result list. If we do not fail, we probably need to make the scaffold result object & the code that manages it more complex.

greg.1.anderson’s picture

I figured out how to fix this without major changes.

+++ b/composer/Plugin/Scaffold/Operations/ReplaceOp.php
@@ -85,10 +85,11 @@ protected function copyScaffold(ScaffoldFilePath $destination, IOInterface $io)
+      $io-writeError($interpolator->interpolate("  - Could not copy [dest-rel-path] from [src-rel-path]"));

The scaffold plugin will do the right thing if the ScaffoldResult has its 'managed' attribute set to 'false'.

I'd recommend flipping the conditional to `if (!$success)`, and then exit early with `return new ScaffoldResult($destination, FALSE);`

greg.1.anderson’s picture

Actually, I think I am wrong in #17 / #18. If we cannot scaffold a file because it was unmodifiable, it should still be reported as "managed" if the file should be managed in non-error conditions.

Setting to "postponed" because we'll need to reroll this after #3092563: Avoid overwriting .htaccess changes during scaffolding > security problem lands.

Mixologic’s picture

Status: Needs work » Postponed

doin the postponing

mglaman’s picture

Just making a note that this is being reported more in the composer slack channel

greg.1.anderson’s picture

Status: Postponed » Needs work

I thought that #3092563: Avoid overwriting .htaccess changes during scaffolding > security problem would land soon, but perhaps not. We could do this in either order. Setting to 'needs work' in case someone wants to push this forward.

Mixologic’s picture

If Im not mistaken, this is still a very critical issue, and almost probably should be marked as such.

mile23’s picture

Priority: Major » Critical
Issue tags: +needs backport to 8.8.x, +Needs followup, +needs documentation updates

Escalating to critical because it means that scaffolding could easily stop working after you've gone through Drupal's installation process. This could result in an inability to perform Composer updates which, for instance, scaffold the settings.php file under sites.

There is probably a workaround, such as chmod-ing the directory in question manually and then re-performing the install/update.

Adding 'needs followup' from #10 and #15.

Also adding 'needs backport to 8.8.x' because, uh, it does. :-)

Patch in #9 still applies, but only tells you that a scaffold op failed. I think that this late in the release-cycle game, it's adequate to report failures. Ideally, a failure like this would also completely halt the build process and refer users to documentation.

The scaffold documentation then needs to offer the workaround.

We can add a follow-up which might wrangle permissions on behalf of users.

mrpauldriver’s picture

Issue tags: -needs documentation updates +Needs documentation updates

Agree this is critical. Inability to install a new Drupal installation with bundled composer templates should never have past muster.

lolandese’s picture

fjgarlin’s picture

Another possible workaround in the composer.json file

    "extra": {
        "drupal-scaffold": {
            "excludes": [
                "sites/default/default.services.yml"
            ],
            "locations": {
                "web-root": "web/"
            }
        },
        ...
mile23’s picture

Status: Needs work » Needs review
StatusFileSize
new6.05 KB
new8.32 KB
new7.34 KB

Just moving this a little bit.

This adds ScaffoldResult::failed(), so the command/handler can then do something with it.

Also adds a permissions-based unit test.

I concentrated on ReplaceOp, but we'll need similar changes for AppendOp and maybe others.

The last submitted patch, 28: 3091285_28_test_only.patch, failed testing. View results

mile23’s picture

StatusFileSize
new16.82 KB
new8.5 KB

AppendOp gets the same treatment.

It would be great if there were a more generalized error-handling system baked in, with something like a RecoverableScaffoldErrorException or something that can be handled.

Also we should add a --force-it-with-a-crowbar option that attempts to chmod the destination, and then attempts to chmod it back to where it was. Only after that doesn't work would it give up.

mile23’s picture

This patch brought to you by 1983: https://youtu.be/eh1dVptApmc

Added this behavior, where we see the problem caused by the hardened files and directories:

$ composer drupal:scaffold

[..]

  - Copy [web-root]/sites/example.sites.php from assets/scaffold/files/example.sites.php
  - Unable to replace [web-root]/sites/default/default.services.yml because of a permissions error.
  - Unable to replace [web-root]/sites/default/default.settings.php because of a permissions error.
  - Copy [web-root]/modules/README.txt from assets/scaffold/files/modules.README.txt
  - Copy [web-root]/profiles/README.txt from assets/scaffold/files/profiles.README.txt
  - Copy [web-root]/themes/README.txt from assets/scaffold/files/themes.README.txt

                                                                  
  [RuntimeException]                                              
  The following destinations were not properly scaffolded:        
  /Users/paul/projects/drupal/sites/default/default.services.yml  
  /Users/paul/projects/drupal/sites/default/default.settings.php  

We fix it with:

$ composer drupal:scaffold --force-changes

[..]

  - Copy [web-root]/sites/default/default.services.yml from assets/scaffold/files/default.services.yml
  - Copy [web-root]/sites/default/default.settings.php from assets/scaffold/files/default.settings.php

This is accomplished as in #7.1: Chmodding our way to success. If that's not enough, then there's little we can do anyway, and we'll see the same error as before. Note that we chmod the files in question, and then attempt to re-chmod them back to what they were before.

Next step is to follow the lead of #3092563-109: Avoid overwriting .htaccess changes during scaffolding > security problem and make different behaviors for composer install versus composer update.

mile23’s picture

StatusFileSize
new44.34 KB
new31.31 KB

Perhaps a patch would be nice.

Edit: Ugh. I got the composer.json changes in the patch. Will re-upload later.

Status: Needs review » Needs work

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

mile23’s picture

Status: Needs work » Needs review
StatusFileSize
new41.29 KB
new1.74 KB

Always nice when phpunit lets you run a test but then run-tests.sh is more strict about namespaces.

mile23’s picture

StatusFileSize
new41.73 KB
new4.05 KB

Adds a --strict mode, which makes the Composer process blow up with a RuntimeException and response code 1 if there were any errors in scaffolding.

This is in addition to the --force-changes command from #31 which tries to use chmod() to work around permissions errors (and then set all the hardening back up).

This means there's a workflow for a user doing updates that looks like this:

  • Perform composer install/update.
  • See a message that something wasn't scaffolded, but otherwise everything's OK.
  • Decide to act on it by either running composer drupal:scaffold --force-changes or doing a chmod and then re-running composer install.

For scripts, we can't add a --strict or --force-changes to composer install, but we can re-perform the scaffolding and look for errors, like this:

  • composer install # normal install, won't error out on a scaffold error. Everything but the failed scaffolded files is installed.
  • composer drupal:scaffold --strict # Re-attempt the scaffold process, will error out on scaffold problem.
  • Now if the scaffolding breaks, your script has a status code of 1 from composer drupal:scaffold --strict and you can figure out what to do next.
larowlan’s picture

nice work 😎

  1. +++ b/composer/Plugin/Scaffold/Handler.php
    @@ -178,6 +194,26 @@ public function scaffold() {
    +      }
    +      else {
    

    🧐nit, don't need the else if we threw an exception in the previous hunk

  2. +++ b/composer/Plugin/Scaffold/Operations/AppendOp.php
    @@ -93,34 +102,49 @@ public function process(ScaffoldFilePath $destination, IOInterface $io, Scaffold
    +      $unhardener = new Unhardener($destination_path);
    

    😂naming is hard. I'd have called it a Softenator

  3. +++ b/composer/Plugin/Scaffold/Operations/AppendOp.php
    @@ -93,34 +102,49 @@ public function process(ScaffoldFilePath $destination, IOInterface $io, Scaffold
    +        $messages = ["  - Could not prepend/append to <info>[dest-rel-path]</info>"];
    

    ❓doesn't this clobber any previously buffered messages?

  4. +++ b/composer/Plugin/Scaffold/Operations/AppendOp.php
    @@ -93,34 +102,49 @@ public function process(ScaffoldFilePath $destination, IOInterface $io, Scaffold
    +      $unhardener->reharden();
    


    we're assuming that force changes implies reharden here - should unharden return the current state and we only reharden if it was previously hard?
    edit: the hardener keeps state, ignore

  5. +++ b/composer/Plugin/Scaffold/Operations/OperationFactory.php
    @@ -26,8 +33,9 @@ class OperationFactory {
    +  public function __construct(Composer $composer, $force_changes) {
    

    ❓should this have a default for BC?

  6. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldResult.php
    @@ -56,4 +64,8 @@ public function destination() {
    +  public function failed() {
    

    🔧missing docs

  7. +++ b/composer/Plugin/Scaffold/Unhardener.php
    @@ -0,0 +1,59 @@
    + * A class to 'unharden' a hardened file system.
    

    🧐nit: 'and reharden'?

  8. +++ b/composer/Plugin/Scaffold/Unhardener.php
    @@ -0,0 +1,59 @@
    +        $this->unhardened[$unharden] = (stat($unharden))['mode'] & 000777;
    +        chmod($unharden, 0744);
    

    ❓this can still fail right? should we be catching those cases where the user has insufficient perms to unharden?

  9. +++ b/composer/Plugin/Scaffold/Unhardener.php
    @@ -0,0 +1,59 @@
    +      foreach ($this->unhardened as $path => $mode) {
    

    ✅ah ignore my comment above, reharden actually tracks the things we unhardened in the first place.

mile23’s picture

StatusFileSize
new42.12 KB
new422.52 KB

Thanks, @larowlan!

Re: #36:

.2 Leaving as Unhardener. :-)

.3 Yes, it clobbers other messages. An AppendOp can perform both prepend and append in one go, so we have to collect either or both of those messages. But if we fail for this object we only want to say that we couldn’t do either.

Did the rest except for .8, because it'll require some cogitation.

mile23’s picture

StatusFileSize
new4.4 KB

Sorry about that. Here's the real diff between #35 and #37.

mile23’s picture

StatusFileSize
new43.92 KB
new4.46 KB

Addressing failed stat() and chown() in the Unhardener class, #36.8.

mile23’s picture

larowlan’s picture

  1. +++ b/composer/Plugin/Scaffold/Unhardener.php
    @@ -4,6 +4,10 @@
    + * This class should not raise errors or throw exceptions. We make a good-faith
    
    @@ -35,12 +39,17 @@ public function __construct($destination_path) {
    +        if ($stat = @stat($unharden)) {
    +          if (@chmod($unharden, 0744)) {
    

    however, could we bubble up a return ?

    e.g if all operations succeeded, return TRUE, otherwise FALSE so the caller at least knows that its their problem now?

  2. +++ b/composer/Plugin/Scaffold/Unhardener.php
    @@ -4,6 +4,10 @@
    + * effort to unharden the file system, and if it does not work the caller is expected
    + * to handle write errors within its scope.
    
    @@ -35,12 +39,17 @@ public function __construct($destination_path) {
    +    // Try to unharden the parent directory first, because that might be why the path
    +    // is unwritable.
    
    @@ -50,8 +59,10 @@ public function unharden() {
    +      // Reverse the array of unhardened paths because we want to change the child
    +      // item before the parent item.
    

    🧐 nit > 80 (can be fixed on commit)

mile23’s picture

StatusFileSize
new46.92 KB
new10.63 KB

Unhardener now tells you about its successes and failures, and you dutifully sit by like a good friend and listen to what it says. :-)

Also some CS/documentation fixes.

greg.1.anderson’s picture

While I like the capabilities this patch offers, I am -1 on the specific implementation. We have to pass the force flag all the way down to the operations, and then duplicate code in append and replace to handle this operation. For strict, we need to remember the failure status in each result object, and set that boolean from each operation.

If instead we first finished the refactoring from #3092563: Avoid overwriting .htaccess changes during scaffolding > security problem, then the unhardener could try to `chmod` unwritable files before the operations themselves were called to attempt to write their contents. Similarly, we could have the ScaffoldFileCollection check and collect the result of each operation's result, and avoid the modifications to the result objects themselves. This would reduce code duplication and fewer new data fields in the operations and result objects, and keep the responsibilities of the unhardener and strictness enforcer more centralized in their respective implementing classes.

If this seems reasonable, I could try to take #3092563 sans the controversial hash file collection and integrate it with this patch tomorrow.

greg.1.anderson’s picture

Another option would be to postpone this issue and doe the idea from #43 in #3102839: Only scaffold files when scaffold package is changed, and re-adjust here after that lands.

mile23’s picture

It was simpler before #41.1. :-)

But yah, it'd be better to have a more integrated approach. The important goals, however, are that the build not break, unless we want it to with --strict or similar.

I considered removing the unhardener parts earlier, because that at least gives us a solution to the critical issue of uncaught exceptions. We could add the nice hand-holding parts in a followup.

greg.1.anderson’s picture

OK, the next thing I'll do, then, is make a patch for #3102839: Only scaffold files when scaffold package is changed (amended as I propose in #3), and reserve judgment about whether we can make an interim solution here before that lands. In theory, if we take out the unhardener, then we'd just be left with an extra boolean in the result objects to remove later, which doesn't sound like much to quibble over. Removing stuff is a b/c break, though, so I'd like to see if we can avoid putting it in in the first place, if possible.

greg.1.anderson’s picture

I posted a patch to #3103090: Avoid re-scaffolding unchanged files (and printing scaffold file information over and over). If we did get that patch committed first, it would make it easier to refactor the code here per #43.

andileco’s picture

What can users do manually to require a new module while we wait for this to be fixed (issue is happening to me on 8.8.1)?

Edit: it looks like everything was updated except composer.json. So I manually added the require line to composer.json, and I think that did what I needed for the time being.

mile23’s picture

The workaround is to use chmod on the hardened files, as outlined here: https://www.drupal.org/docs/develop/using-composer/starting-a-site-using...

binnythomas’s picture

The solution @50 worked for me. Thanks

jannakha’s picture

Workaround in #27 almost correct, but it should be:

"drupal-scaffold": {
      "locations": {
        "web-root": "docroot/"
      },
      "file-mapping": {
        "[web-root]/sites/development.services.yml": false
      }
    },

as of composer scaffolding documentation

that worked for me on Drupal 8.8.2

rohitreddygaddam’s picture

As per @fjgarlin This works fine, however not sure if this is the recommended way

 "extra": {
        "drupal-scaffold": {
            "excludes": [
                "sites/default/default.services.yml"
            ],
            "locations": {
                "web-root": "web/"
            }
        },
mmjvb’s picture

@rohitreddygaddam Not recommended. This issue is for drupal/core-composer-scaffold. The `excludes` key under `drupal-scaffold` is from drupal-composer/drupal-scaffold. Doubt very much it works for drupal/core-composer-scaffold.

kris77’s picture

The solution in #50 works for me too.

Now I always see this lines:
Scaffolding files for drupal/core:
- Copy [project-root]/.editorconfig from assets/scaffold/files/editorconfig
- Copy [project-root]/.gitattributes from assets/scaffold/files/gitattributes
- Copy [web-root]/.csslintrc from assets/scaffold/files/csslintrc
- Copy [web-root]/.eslintignore from assets/scaffold/files/eslintignore
- Copy [web-root]/.eslintrc.json from assets/scaffold/files/eslintrc.json
- Copy [web-root]/.ht.router.php from assets/scaffold/files/ht.router.php
- Copy [web-root]/.htaccess from assets/scaffold/files/htaccess
- Copy [web-root]/example.gitignore from assets/scaffold/files/example.gitignore
- Copy [web-root]/index.php from assets/scaffold/files/index.php
- Copy [web-root]/INSTALL.txt from assets/scaffold/files/drupal.INSTALL.txt
- Copy [web-root]/README.txt from assets/scaffold/files/drupal.README.txt
- Copy [web-root]/robots.txt from assets/scaffold/files/robots.txt
- Copy [web-root]/update.php from assets/scaffold/files/update.php
- Copy [web-root]/web.config from assets/scaffold/files/web.config
- Copy [web-root]/sites/README.txt from assets/scaffold/files/sites.README.txt
- Copy [web-root]/sites/development.services.yml from assets/scaffold/files/development.services.yml
- Copy [web-root]/sites/example.settings.local.php from assets/scaffold/files/example.settings.local.php
- Copy [web-root]/sites/example.sites.php from assets/scaffold/files/example.sites.php
- Copy [web-root]/sites/default/default.services.yml from assets/scaffold/files/default.services.yml
- Copy [web-root]/sites/default/default.settings.php from assets/scaffold/files/default.settings.php
- Copy [web-root]/modules/README.txt from assets/scaffold/files/modules.README.txt
- Copy [web-root]/profiles/README.txt from assets/scaffold/files/profiles.README.txt
- Copy [web-root]/themes/README.txt from assets/scaffold/files/themes.README.txt

but I can install modules via composer.

Thanks @Mile23

yktdan’s picture

#52 does not work for me on 8.8.4. The document page https://www.drupal.org/docs/develop/using-composer/starting-a-site-using... says it will be fixed in 8.8.1 but clearly it is still broken.
At this point I am pretty sure my lock file is out of sync with json and I don't know how to fix that. But the whole lock file seems like a bad design as it seems to grow unbounded and will slow things down as it gets larger. In my experience have two files that must stay in sync is a non-starter when the system can crash at any time guaranteeing that they can get out of sync.

I also don't understand who fixes composer.json if I patch it as in #52. What happens when the bug is really fixed?

mmjvb’s picture

@yktdan Suggest to remove the scaffold plugin as it currently doesn't work properly.

You can use `composer update --lock` to synchronize the json with lock.

The composer.json is a specification of your project. You are responsible for maintaining it. Obviously, when this `bug` is fixed you may need remove your change. You may decide to use the plugin again, depending on how it is fixed.

yktdan’s picture

Thanks, but I have not found instructions on removing scaffolding. Can I just composer uninstall drupal/core-composer-scaffold , but it is in the core part of requires. So perhaps I just remove that line and do an update?

mmjvb’s picture

Use `composer remove drupal/core-composer-scaffold`. It is not in the core part, it is part of the template used for your project.

yktdan’s picture

How do I know when it is safe to put scaffolding back in? Just upgraded 8.8.4 to 8.8.5 but that is not where the changes are (or maybe a real fix requires core changes?). It is not obvious (to me at least) the effect of not updating scaffolding if a security fix comes out in one of the scaffolding files.

mmjvb’s picture

You know by checking how this issue is fixed when it appears in the release notes of a maintenance release. That is assuming it gets backported to D8.8.

Suggest to stick with the proper procedure validating changes to scaffold files and applying them where needed. It looks like there is no intention to make the scaffold plugin do that for you.

greg.1.anderson’s picture

The related issue, #3103090: Avoid re-scaffolding unchanged files (and printing scaffold file information over and over), reduces the number of times a scaffold file is rewritten, which resolves nearly all of the issues addressed by the patches here. That issue is RTBC. Efforts to get that committed (more testing / feedback / +1s) would be a productive way to help address the problems being experienced here.

Removing the scaffolding plugin is a pretty poor choice as far as potential workarounds. Folks who do this will be responsible for noticing on their own when scaffold files change during an upgrade. Missing changed files has the potential for causing a site to miss out on a security fix, or perhaps even break a site.

The workaround in #52 is a much more prudent option. It disables the scaffold plugin for only the file that is causing problems.

mmjvb’s picture

Removing the scaffold plugin after first use stops all the bad things it does. Advice against selectively disabling bad things. Suggest to adjust the plugin to do the right things in the first place. In which case there is no need to remove it.

You would still need to follow proper procedure, due to the fact that any automation is doing the wrong thing.

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.

thursday_bw’s picture

Status: Needs review » Needs work

Switching this to needs work. At least one of the patches that was affecting the outcome of this is closed now.
The other has moved on significantly.

A couple of follow on issues have been created too.

That other issues being fixed mitigates many of the issues here, it's still a much nicer developer experience to not have to
deal with it at all. It's particularly bad for junior devs and newbies trying to learn a bunch of other stuff and not needing to fight with
composer.

So I vote for further disuccion for how to manager the scaffolding cleanly and transparently. Mitigating is great, but we can do more.

thursday_bw’s picture

Issue tags: +Bug Smash Initiative
greg.1.anderson’s picture

#3103090: Avoid re-scaffolding unchanged files (and printing scaffold file information over and over) has been merged. This should take care of this symptoms described in this issue in most instances, as least when using drupal/{recommended,legacy}-project without any additional scaffold files. In that scenario, the only time the failure reported in this issue should be seen is:

1. The site builder modifies sites/default/default.services.yml or sites/default/default.settings.php. (Should be rare, as these files should be copied before they are modified.)
2. The site builder removes sites/default/default.services.yml or sites/default/default.settings.php.

Doing either of these things will cause the next scaffold operation to fail. The best workaround today is still #52 (although n.b. the filename is wrong in that comment).

We could still continue with this patch in order to improve the scaffolding operation such that these workarounds are not necessary. If someone wanted to move this forward, my comments in #43 still apply. Rather than adding so many parameters to control the behavior of the replace and append ops, perhaps a simpler strategy of simply trying harder would be appropriate. i.e. the scaffold plugin could always try to make a write-protected file writable first, and only fail if it does not have permission to do that.

Another enhancement we could consider would be to add an OPTIONAL attribute to scaffold files. If a scaffold file is optional, then any attempt to modify the file that fails becomes a warning instead of an error. We could then mark scaffold files such as editorconfig, README.txt, and the above-mentioned default files as optional. This idea could also be done in a follow-on issue.

thursday_bw’s picture

Title: Permissions hardening prevents Composer scaffolding from copying, causes composer require to fail » Composer scaffolding fails when permissions or default.settings.yml or default.settings.php is not writable.
Issue summary: View changes
thursday_bw’s picture

Title: Composer scaffolding fails when permissions or default.settings.yml or default.settings.php is not writable. » Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable.
thursday_bw’s picture

Priority: Critical » Normal
Issue summary: View changes
thursday_bw’s picture

After discussions with Greg, Larowlan in slack
Just dropped the priority from critical to normal

https://drupal.slack.com/archives/C392CHBEW/p1591746380039400

greg.1.anderson’s picture

Issue summary: View changes

Clarified summary.

weibing82’s picture

The current workaround did not work in drupal 8.9.3

The file path in the current workaround should be:

"drupal-scaffold": {
      "locations": {
        "web-root": "docroot/"
      },
      "file-mapping": {
        "[web-root]/sites/default/default.services.yml": false,
        "[web-root]/sites/default/default.settings.php": false
      }
    },

ps. If you use the drupal/recommended-project template, the locations defined should be "web-root":"web/".

robcarr’s picture

Addition of the 'file-mapping' in composer.json (as described in #73) seemed to work for me in 8.9.3. Thanks for the help

paddyilos’s picture

#73 work for me as well. however, I am using Acquia Dev Desktop, so "web-root":"./"

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

bramvandenbulcke’s picture

This is a tough issue. The error "Installation failed, reverting..." is not giving any indication what it is about.

Luckily, #73 is working for me. I'm using "web-root": "web/" on a MAMP drupal-recommended installation.

vlad.dancer’s picture

Thanks @weibing82
#73 Works for us too.

superlolo95’s picture

#73 Works for me too.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

rovo’s picture

After scouring the web via google, I finally came across this solution; so glad I did. This fixed it for me #73 fixed it for me also!

nathan tsai’s picture

#73 worked for me.

Before composer.json

  "extra": {
    "drupal-scaffold": {
      "locations": {
        "web-root": "./",
        "file-mapping": {
            "[web-root]/sites/default/default.services.yml": false,
            "[web-root]/sites/default/default.settings.php": false
        }
      }
    },
    ...
  }

After composer.json

  "extra": {
    "drupal-scaffold": {
      "locations": {
        "web-root": "./"
      },
      "file-mapping": {
        "[web-root]/sites/default/default.services.yml": false,
        "[web-root]/sites/default/default.settings.php": false
      }
    },
    ...
  }

Previously was getting the error:

> post-install-cmd: Drupal\Composer\Plugin\Scaffold\Plugin->postCmd


  [ErrorException]
  is_dir() expects parameter 1 to be a valid path, array given


Exception trace:
 () at phar:///opt/cpanel/composer/bin/composer/src/Composer/Util/Filesystem.php:180
 Composer\Util\ErrorHandler::handle() at n/a:n/a
 is_dir() at phar:///opt/cpanel/composer/bin/composer/src/Composer/Util/Filesystem.php:180
 Composer\Util\Filesystem->ensureDirectoryExists() at /home/MYSITE/public_html/vendor/drupal/core-composer-scaffold/ManageOptions.php:85
 Drupal\Composer\Plugin\Scaffold\ManageOptions->Drupal\Composer\Plugin\Scaffold\{closure}() at n/a:n/a
 array_map() at /home/MYSITE/public_html/vendor/drupal/core-composer-scaffold/ManageOptions.php:84
 Drupal\Composer\Plugin\Scaffold\ManageOptions->ensureLocations() at /home/MYSITE/public_html/vendor/drupal/core-composer-scaffold/ManageOptions.php:73
 Drupal\Composer\Plugin\Scaffold\ManageOptions->getLocationReplacements() at /home/MYSITE/public_html/vendor/drupal/core-composer-scaffold/Handler.php:153
 Drupal\Composer\Plugin\Scaffold\Handler->scaffold() at /home/MYSITE/public_html/vendor/drupal/core-composer-scaffold/Plugin.php:102
 Drupal\Composer\Plugin\Scaffold\Plugin->postCmd() at n/a:n/a
 call_user_func() at phar:///opt/cpanel/composer/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php:173
 Composer\EventDispatcher\EventDispatcher->doDispatch() at phar:///opt/cpanel/composer/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php:101
 Composer\EventDispatcher\EventDispatcher->dispatchScript() at phar:///opt/cpanel/composer/bin/composer/src/Composer/Installer.php:345
 Composer\Installer->run() at phar:///opt/cpanel/composer/bin/composer/src/Composer/Command/InstallCommand.php:131
 Composer\Command\InstallCommand->execute() at phar:///opt/cpanel/composer/bin/composer/vendor/symfony/console/Command/Command.php:245
 Symfony\Component\Console\Command\Command->run() at phar:///opt/cpanel/composer/bin/composer/vendor/symfony/console/Application.php:835
 Symfony\Component\Console\Application->doRunCommand() at phar:///opt/cpanel/composer/bin/composer/vendor/symfony/console/Application.php:185
 Symfony\Component\Console\Application->doRun() at phar:///opt/cpanel/composer/bin/composer/src/Composer/Console/Application.php:310
 Composer\Console\Application->doRun() at phar:///opt/cpanel/composer/bin/composer/vendor/symfony/console/Application.php:117
 Symfony\Component\Console\Application->run() at phar:///opt/cpanel/composer/bin/composer/src/Composer/Console/Application.php:122
 Composer\Console\Application->run() at phar:///opt/cpanel/composer/bin/composer/bin/composer:63
 require() at /opt/cpanel/composer/bin/composer:24

install [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-suggest] [--no-dev] [--no-autoloader] [--no-scripts] [--no-progress] [--no-install] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--apcu-autoloader-prefix APCU-AUTOLOADER-PREFIX] [--ignore-platform-req IGNORE-PLATFORM-REQ] [--ignore-platform-reqs] [--] [<packages>]...

Marsell key made their first commit to this issue’s fork.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sjhuskey’s picture

This is still an issue. I ran into this issue today on a 9.4.3 site. #73 solved the problem for me.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gajanannehul’s picture

I ran into same issue on a 9.4.3 Drupal version and #73 saved my day.

noah’s picture

#73 is working for me with Drupal 9.5.9, however I saw something else that I couldn't find addressed anywhere else that I thought was worth adding here in case anyone else encounters the error I did. I moved a site to a new environment with this in the composer.json:

    "extra": {
        "drupal-scaffold": {
            "locations": {
                "web-root": "web/"
            },
            "file-mapping": {
                "[web-root]/sites/development.services.yml": "false"
            }
        }

...and when I did the initial composer update, scaffolding failed with the error:

In ScaffoldFilePath.php line 135:
                                                                        
  Scaffold file false not found in package drupal/recommended-project.

I removed the file-mapping and and did composer update, and it completed successfully (including scaffolding). I put file-mapping back and subsequent updates worked as well with no errors. So I think it's just a matter of having to exclude this for an initial install.

UPDATE: Please disregard this, I just realized that it's the quotes around "false" in the file mapping that were causing the problem. I didn't get any results for the error message I got, so I'll leave the rest of this in case someone else makes a similar mistake. Apologies for the noise.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

wim leers’s picture

@tedbow discovered this is also getting in the way of Automatic Updates: #3362143-21: Use the rsync file syncer by default.

wim leers’s picture

Issue summary: View changes

AFAICT the current issue summary is wrong?

It says:

the only time the failure reported in this issue should be seen is:

  1. The site builder modifies sites/default/default.services.yml or sites/default/default.settings.php. (Should be rare, as these files should be copied before they are modified.)
  2. The site builder removes sites/default/default.services.yml or sites/default/default.settings.php.

But it's AFAICT also a problem whenever core updates one of these files.

The last 10 times that sites/default/default.settings.php was changed:

$ git log --pretty=format:"%ad%x09%<(25,trunc)%s" -n 10 origin/10.1.x -- sites/default/default.settings.php
Wed Apr 19 11:18:37 2023 -0500  SA-CORE-2023-005 by ben..
Tue Apr 11 14:10:23 2023 +0100  Issue #3027639 by catch..
Sun Mar 12 20:06:51 2023 +0000  Issue #3107548 by tunic..
Thu Feb 23 16:22:19 2023 +0000  Issue #3317265 by ressa..
Thu Feb 16 22:36:17 2023 +0000  Issue #3333281 by Musta..
Wed Nov 30 17:35:34 2022 +0000  Issue #3032746 by mfb, ..
Thu Nov 17 14:13:32 2022 +0000  Issue #3260401 by idebr..
Mon Oct 3 14:17:48 2022 +0100   Issue #3096101 by quiet..
Mon Aug 8 11:39:05 2022 +0300   Issue #3262674 by tstoe..
Wed Jul 20 10:11:30 2022 -0500  SA-CORE-2022-012 by cml..

👆 5 times in the past first 5 months of 2023!

Same for sites/default/default.services.yml:

$ git log --pretty=format:"%ad%x09%<(25,trunc)%s" -n 10 origin/10.1.x -- sites/default/default.services.yml
Mon Mar 6 17:14:57 2023 +0000   Issue #3150614 by pfren..
Fri Mar 3 16:08:14 2023 +0000   Revert "Issue #3150614 ..
Fri Mar 3 11:13:53 2023 +0000   Issue #3150614 by pfren..
Thu Feb 23 10:20:36 2023 +0000  Issue #3198868 by dpi, ..
Sun Oct 9 12:06:21 2022 +0100   Issue #3112452 by lalit..
Mon Oct 3 14:30:44 2022 +0100   Issue #3305748 by kay_v..
Wed Sep 28 11:52:42 2022 -0500  SA-CORE-2022-016 by fab..
Wed Sep 21 14:49:58 2022 +0100  Issue #2381797 by Tom V..
Mon Feb 14 17:23:42 2022 +0000  Issue #3166449 by ravi...
Wed Aug 18 09:53:24 2021 +0100  Issue #2473875 by znero..

👆 4 times in the first 5 months of 2023!

AFAICT that means the disruption is much bigger?

wim leers’s picture

Priority: Normal » Major
Issue tags: +DX (Developer Experience)

The data in #92 + the fact this has 125 followers makes me believe this deserves a priority bump.

xmacinfo’s picture

The problem here is the main default folder permission, as in sites/default/default.services.yml.

We need to somehow let Composer deal with the permission of the default folder. For example, if I manually set sites/default to 777, Composer should be able to run and scaffold the files like default.services.yml or default.settings.php.

Please note that this issue summary mentions this in the historical context section:

If you've installed and run Drupal, the sites/default directory receives permission hardening. This can break the scaffolding plugin.

It's not recommended to run Composer as root. But is there a way to let Composer change the permission of the default folder temporarily, without introducing any security holes? Should Composer scaffolding be responsible for the permission hardening and let Drupal check only if the default folder is hardened?

wim leers’s picture

Interesting, @xmacinfo! I like your proposal! 🤓

Maybe a solution could indeed be to let Drupal's Scaffold plugin not apply the scaffolding for non-essential files, such as sites/default/default.* if file system permissions do not allow Composer to overwrite them.

(I say these are non-essential files because they do not affect the live site in any way, they merely serve as starting points.)

xmacinfo’s picture

OK. I like that as well.

We let Drupal do its own hardening while we modify the Composer scaffold plugin to :

not apply the scaffolding for non-essential files, such as sites/default/default.* if file system permissions do not allow Composer to overwrite them.

Should the Composer scaffold plugin display a notice when it skips a file due to insufficient permissions?

tedbow’s picture

Another solution to this problem is to rethink the need for the sites/default/default.* files all together.

For instance for defaults.settings.php is used by the installer to make settings.php and core/Install.txt has manual instructions to use this file to create settings.php. But all this logic was made before we had core/assets/scaffold/files/default.settings.php.

It would be much easier for the installer to just use core/assets/scaffold/files/default.settings.php.
and core/Install.txt to be updated to reference this file.

Then sites/default/default.settings.php could removed entirely and the scaffold would not have the problem of trying to move the file into folder that should be write protected.

This would make Automatic updates much easier as we would not have try to make this folder writable and then re-harden it again which seem dangerous if for some reason we get some kind of hard failure and can't re-harden it.

It seems like it is only for legacy reasons that we have both sites/default/default.settings.php and and core/assets/scaffold/files/default.settings.php and we have to have a test make sure they are identical(i think)

We could even make a new
sites/default/default.settings.txt
that just explains what happened for anybody looking for the file in the old location.

voleger’s picture

berdir’s picture

Doesn't using scaffold files directly pretty much defeat the point of having scaffold files in the first place, aka the ability to customize them.

Also, I don't really understand how that would solve the problem, the problem with permissions is the target file, not the source file, and as long as we need to copy in the same folder, we still have this problem?

mstrelan’s picture

@Berdir I think what #97 is saying is that sites/default/default.settings.php is used as a reference and we would instead refer people to core/assets/scaffold/files/default.settings.php. You would still have sites/default/settings.php which you could customise, and we would never need to copy default.settings.php in to sites/default.

tedbow’s picture

re #99

Doesn't using scaffold files directly pretty much defeat the point of having scaffold files in the first place, aka the ability to customize them.

I don't think that is only purpose the scaffolding.

From https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...

The purpose of scaffolding files is to allow Drupal sites to be fully managed by Composer, and still allow individual asset files to be placed in arbitrary locations. The goal of doing this is to enable a properly configured composer template to produce a file layout that exactly matches the file layout of a Drupal 8.7.x and earlier tarball distribution.

But I agree my idea in #97 defeats the purpose of site/default/default.* files being in core/assets/scaffold/files because I was proposing never copying these files into another location at all.

So I guess the question is do people really need files like default.setitngs.php to be customized? I am not sure sites want customize the file used as a starting point that settings.php is made from by the installer.

I think that default.settngs.php is scaffold file at all for legacy reasons and the other purpose in the doc for the scaffold is

Other file layouts are also possible; for example, a project layout very similar to the now deprecated drupal-composer/drupal-project template is provided as part of the Drupal Recommended Project composer template.

To customize the contents of default.setitngs.php so the installer would use the customize version of default.settngs.php to make settings.php the project would have to add "[web-root]/sites/default/default.settings.php": "custom_scaffold_files/default.settings.php",. Is that really a use case that is important to support?

The scaffold does allow moving files into different locations but if anyone customized the location of default.setitngs.php to anywhere else other than sites/default I think the installer would break because install_check_requirements has this relative location hardcoded.

So it is possible that people to customize the contents of default.setitngs.php but seems unlikely. It doesn't seem possible that you can change the location of default.setitngs.php because of install_check_requirements.

I think default.setitngs.php right now is a scaffold file at all because we want it to be in same directory as settings.php should be in and the scaffold is how we move files that are in different locations with different composer project layouts. But is that really a necessity default.settings.php live at sites/default? It does certainly make things more difficult that the scaffold has to write to sites/default and we also automatically harden that location. Isn't the more common case the installer itself uses default.setitngs.php to make settings.php and in that case we tell the installer it is anywhere.

So why not not just make new location like core/assets/defaults that would contain default.settings.php and default.services.yml? Then just update the installer to use the new location and update core/install.txt

If there are BC concerns for Drupal 10 we could just use core/assets/scaffold/files/default.settings.php for now but remove from being moved and in Drupal 11 use core/assets/defaults/default.settings.php

wim leers’s picture

A related idea: if sites/default/default.*:

  1. are never used (as in executed)
  2. must always be up-to-date
  3. are present in that specific location only for ease-of-use

… then why don't we symlink them from core/assets/scaffold/files/default.*? That way, they're always up-to-date, and we never have to deal with permissions ever again.

🤔

Although it appears that symlinks do not have their own permissions (they automatically inherit the permissions of the target), there is one exception apparently: macOS. Investigation needed.

wim leers’s picture

So why not not just make new location like core/assets/defaults that would contain default.settings.php and default.services.yml? Then just update the installer to use the new location and update core/install.txt

This sure sounds like a simple and logical solution to me 😄 Probably the simplest solution possible.

Curious to hear what the concerns would be for that proposal!

berdir’s picture

> So I guess the question is do people really need files like default.setitngs.php to be customized?

I doubt it's done often, but I can think of use cases, some kind of distribution/template could allow to customize default.settings.php, but I guess then you could also just provide a settings.php in the first place then.

default.services.yml is not copied by default though, it's really just an example, so not sure if that's easy to find then.

Could indeed make sense to do that, but no idea in what form that's a BC break and when we are allowed to make that change, could also have quite a bit impact on documentation that needs to updated.

It would IMHO be easier to just rip out the logic around the permission check entirely, that was also proposed in the D7 backport issue of the skip_permissions_hardening setting IIRC.

xmacinfo’s picture

Another solution:

Have Composer scaffold core/assets/defaults/default.settings.php to sites/templates/default.settings.php.

This new sites/templates folder would not be executable and contain only templates or examples files.

We could use examples instead, too. So either:

sites/templates/default.settings.php
sites/examples/default.settings.php

mglaman’s picture

Honestly, it'd be great if we didn't have to have sites/default/default.settings.php as a requirement. No symlink, so copy. It makes running Drupal out of the vendor directory that much easier.

I still think it makes sense to have Composer chmod 775 sites/default.

andypost’s picture

templates approach will lead to outdated files and more errors on updates because user's will forget to update files

xmacinfo’s picture

templates approach will lead to outdated files and more errors on updates because user's will forget to update files

I would expect Composer to scaffold the files inside templates. Thus all those files, managed by Composer scaffold, inside the template folder, will always be up-to-date.

I suggest to scaffold the default.settings.php file in sites/templates instead of sites/default. Same mechanism. So no outdated files.

omd’s picture

Just a clarification:

do we actually use the exact path docroot/ in:

"drupal-scaffold": {
"locations": {
"web-root": "docroot/"
},
"file-mapping": {
"[web-root]/sites/default.services.yml": false,
"[web-root]/sites/default.settings.php": false
}
},

Or is that meant to be a placeholder for the current document root of your installation, which for me is web/. I've tried both and using docroot/ creates a whole new drupal installation in a new folder called docroot, which didn't seem right but the error stopped. I also tried I using web/ which seems right but I still get the "could not delete" error messages .

anybody’s picture

+1 on #103. Not sure if the templates or the defaults terminology fits better.
I think a default is something, that's used "by default" and is kind of required and "active"
While a template is best-practice that can be used but is not actively used

The risk of #107 is true, but the opposite is overwriting modifications?
Best might be to have an inheritance logic, but that may also lead to collisions with overwrites / customizations.
No Silver Bullet. What's best and how can we figure it out?

ressa’s picture

Priority: Major » Critical
Issue summary: View changes

This is critical, and this issue is getting close to its four year birthday.

Following the officially recommended method of Updating Drupal core via Composer to deploy a new composer.lock file on the production server with composer install can get you a critical error, and a broken web site, until you understand what the problem is (finding this page), implementing the workaround here, finding the commands to relax permissions, complete the update, and then resetting file permissions and ownership again, hoping it doesn't happen next time.

What should be a boring and uneventful task becomes a painful experience. The next update will be dreaded.

+1 for moving sites/default/default.settings.php elsewhere as @tedbow and @mglaman suggest, if that solves the problem. If it also makes work on Automatic Updates easier, that's another strong argument for this proposal.

xmacinfo’s picture

Another solution is to remove the whole default.settings.php file altogether and document the default values in the README.md file.

But if we want to keep that file, we can rename it to default.settings.txt so that PHP will not try to execute it.

Even if we only rename the file, we need Composer Scaffold to store that file outside of the default protected folder.

ericdsd’s picture

Hi xmacinfo, note that renaming a php file to txt, makes it readable which is imho a lot worse.

some solutions could be :
- scaffolding in a dedicated directory that would remain writable eg. sites/examples/ as suggested in #111 (proper name might be discussed)
- not scaffolding it (eventually add a composer command to copy it when needed, or displaying a note telling where core example assets can be copied from)

vakulrai’s picture

Thees 2 files should be left alone while working as they only serve the purpose to bring the most updated changes thats comes with core and usually should be copied and the worked upon in case of any integration ,I usually follow this while working on any project.

sites/default/default.services.yml
sites/default/default.settings.php

I came across the problem of permission hardening while moving from 9.5.x to 10.x and i feel + 1 to modify the scaffolding process in a manner that if the permission of incoming version differs from what is locally present try to do the below:

  • Store the existing permission and do a chmod 775 on sites/default
  • replace or overwrite based on the scaffolding process set in composer.json
  • Then restore the existing permission from bullet 1

Else fallback to the default behaviour.

emersonreis.dev’s picture

I'm coming from https://www.drupal.org/drupalorg/blog/introducing-the-bounty-program and just read everything so I'd like to make a summary of the current state of this issue. First of all, I'm somewhat "ignoring" the comments and patches before #67 as they happened before 3103090 was implemented.

First of all, these are the scenarios that could cause this issue to happen:

  1. The site builder modifies the files (#67)
  2. The site builder removes the files (#67)
  3. Core updates the files (#92)

These are the workarounds:

1. composer.json changes

Documentation: https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...
Code:

"extra": {
  "drupal-scaffold": {
    "file-mapping": {
      "[web-root]/sites/default/default.services.yml": false,
      "[web-root]/sites/default/default.settings.php": false
    }
  }
}

See #73.

2. chmod

As explained on https://www.drupal.org/docs/develop/using-composer/starting-a-site-using...

You can simply chmod before running the composer command you want to run.

The following are the suggestions to fix the issue:

1. Trying harder

Suggested by greg.1.anderson on #67:

a simpler strategy of simply trying harder would be appropriate. i.e. the scaffold plugin could always try to make a write-protected file writable first, and only fail if it does not have permission to do that.

My thoughts

It's a good idea, but we risk leaving the folder/file writable if the composer command fails in the middle of the process, I know it's unlikely, but imagine a scenario where we change the folder/file permission to writable and composer fails because of OOM before reverting that change, if you try to run the command again it will just succeed but it won't "unharden" (change the permissions back to what they were before) as it won't know what permissions they were before.

2. Optional scaffold files

Suggested by greg.1.anderson on #67:

If a scaffold file is optional, then any attempt to modify the file that fails becomes a warning instead of an error. This idea could also be done in a follow-on issue.

#95 also metions the same solution.

My thoughts

I like the idea and I think it also links to the next suggestion.

3. Don't even try

Suggested by tedbow on #97:
I can't find a quote so here's a summary: He suggests rethinking the need for scaffolding sites/default/default.* files to avoid folder permission issues and facilitate the Automatic Updates initiative.

TLDR: Don't scaffold these files at all.

My thoughts

I like this idea. There is a concern raised about this though which is cases where the Drupal installation is actually using that file.

4. Symlink

Suggested by Wim Leers on #102:

why don't we symlink them from core/assets/scaffold/files/default.*? That way, they're always up-to-date, and we never have to deal with permissions ever again.

My thoughts

I like this idea but the same concern from the previous suggestion applies here I believe, if an installation is using the file how is it gonna update the contents of it if it's not actually a file, only a link? Developers have the option to append files as part of the scaffolding process as well.

5. Templates folder

Suggested by xmacinfo on #105:

Have Composer scaffold core/assets/defaults/default.settings.php to sites/templates/default.settings.php.

My thoughts

We need to remember that the sites directory is used by multisites Drupal installations and that would mean if there is an installation called "examples" or "templates" they would now have new files into their folder "out of nowhere".

6. md or txt

Suggested by xmacinfo on #112:

remove the whole default.settings.php file altogether and document the default values in the README.md file

if we want to keep that file, we can rename it to default.settings.txt so that PHP will not try to execute it.

we need Composer Scaffold to store that file outside of the default protected folder.

My thoughts

Someone else raised the concern about using those two extensions that they might become available to end-users, as webservers are usually configured to block .php files but to allow .md and .txt files, so this comes with security risks attached.

My Suggestion

Do what suggestion number 3 is saying, but because it's possibly not BC for some use case scenarios, do what suggestion number 2 is saying for now until we release a new major version (Drupal 11?), have a list of files that are "optional" (naming to be discussed), try to modify them, but don't fail the whole process, only display a deprecation warning linking to a piece of documentation of why this warning is being displayed and what can be done to get rid of it.
This means we would have to update all the references to these files, such as the core/Install.txt file and others.

Another potential solution would be to simply do suggestion 2 as part of this issue, and the rest as part of a follow-up. Just to make sure we resolve the actual problem of devs not being able to run composer commands as soon as possible.

vakulrai’s picture

Status: Needs work » Needs review

I tried to reproduce it on 10.x and 11.x branch by doing the below changes but the issue is not happening anymore:

  • I intentionally added chmod 444 to sites/default/default.settings.php file while on a 9.5.x branch
  • Moved to Drupal 10.x branch and run composer update but what i can see is that the 444 got converted to 644

I think this ReplaceOp::process() method is managing the files in a correct manner making sure the permission should not block any incoming content.

/**
   * {@inheritdoc}
   */
  public function process(ScaffoldFilePath $destination, IOInterface $io, ScaffoldOptions $options) {
    $fs = new Filesystem();
    $destination_path = $destination->fullPath();
    // Do nothing if overwrite is 'false' and a file already exists at the
    // destination.
    if ($this->overwrite === FALSE && file_exists($destination_path)) {
      $interpolator = $destination->getInterpolator();
      $io->write($interpolator->interpolate("  - Skip <info>[dest-rel-path]</info> because it already exists and overwrite is <comment>false</comment>."));
      return new ScaffoldResult($destination, FALSE);
    }

    // Get rid of the destination if it exists, and make sure that
    // the directory where it's going to be placed exists.
    $fs->remove($destination_path);
    $fs->ensureDirectoryExists(dirname($destination_path));
    if ($options->symlink()) {
      return $this->symlinkScaffold($destination, $io);
    }
    return $this->copyScaffold($destination, $io);
  }
needs-review-queue-bot’s picture

Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

vakulrai changed the visibility of the branch 3091285-composer-scaffolding-fails-preprocess to hidden.

vakulrai’s picture

Status: Needs work » Needs review

The branch has merge conflicts can someone rebase it , please review the small check i added to Drupal\Composer\Plugin\Scaffold\Operations\ReplaceOp::process.

Thanks !

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

trackleft2 made their first commit to this issue’s fork.

nmudgal made their first commit to this issue’s fork.

nmudgal’s picture

Status: Needs work » Needs review

Fixed failing test cases.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.79 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nmudgal’s picture

Status: Needs work » Needs review
ressa’s picture

Issue summary: View changes

Updating Current workaround, to match the default paths:

"drupal-scaffold": {
    "locations": {
        "web-root": "web/"
    },
    "file-mapping": {
        "[web-root]/sites/default/default.services.yml": false,
        "[web-root]/sites/default/default.settings.php": false
    }
},
smustgrave’s picture

Can issue summary be updated. Mentioned 2 options but not sure which.

neograph734’s picture

I have not seen anybody make mention of the following approach, which appears to be working for me:

{
    ...

    "scripts": {
        "pre-drupal-scaffold-cmd": [
            "chmod 775 web/sites/default"
        ],
        "post-drupal-scaffold-cmd": [
            "chmod 555 web/sites/default"
        ]
    },
    "extra": {
        "drupal-scaffold": {
            "locations": {
                "web-root": "web/"
            }
        },
    ...
}

This allows the scaffolding of all files to take place without having to do the chmod manually and without skipping any files.

xmacinfo’s picture

You are into something! Thanks!

So core would not need any changes. But the user, with that solution, can change his root composer.json file to fix this issue.

smustgrave’s picture

Should this be moved to NW then?

xmacinfo’s picture

Well…

  • NW — If we want to modify the Core composer file or any related scaffold files.
  • Close (won’t fix) — If we only want to document how to use that Drupal Composer scaffold command #131.

Personally, I suggest to document this in https://www.drupal.org/docs/develop/using-composer/using-drupals-compose....

The post-drupal-scaffold-cmd can be very powerful:

"post-drupal-scaffold-cmd": [
  "cd docroot && git apply -v ../patches/my-htaccess-tweaks.patch"
]
smustgrave’s picture

If others agree we can go that approach!

xmacinfo’s picture

Composer output without using the pre-drupal-scaffold-cmd and post-drupal-scaffold-cmd scripts:

Scaffolding files for drupal/core:

In Filesystem.php line 303:

  Could not delete /Users/jrblier/web/Sites/2017.local.xmacinfo.com/web/sites/default/default.services.yml:

Here is the composer output when using #131:

…
Scaffolding files for drupal/core:
  - Copy [web-root]/sites/default/default.services.yml from assets/scaffold/files/default.services.yml
> chmod 555 web/sites/default
> DrupalProject\composer\ScriptHandler::createRequiredFiles
…
xmacinfo’s picture

Issue summary: View changes

I did update the Issue Summary Current workaround section.

xmacinfo’s picture

Issue summary: View changes
xmacinfo’s picture

Priority: Critical » Normal

One less critical.

catch’s picture

Status: Needs review » Needs work

Thanks for the issue summary updates. I'm still findinf this issue a bit hard to follow since it feels like it's covering multiple situations.

It sounds like there is a remaining issue where core changes to these files will still fail scaffolding, and I think it would make sense to change that to a warning rather than a failure.

We might need a spin-off issue to then explore some of the other approaches to remove the warning.

prudloff changed the visibility of the branch 11.x to hidden.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.