Use the rsync file syncer by default

If this were in, we could do more in 📌 Warn strongly if the rsync file syncer is not in use Fixed .

Now the patch will apply.

#5:

E: Unable to locate package rsync

Per http://grep.xnddx.ru/search?text=apt-get&filename=drupalci.yml, I'd expect this to work fine 🤓

#8:

-- Commands Executed ---
sudo apt-get update
sudo apt-get install rsync
modules/contrib/automatic_updates/scripts/commit-code-check.sh --drupalci
Return Code: 1
--- Output ---
Get:1 http://deb.debian.org/debian bullseye InRelease [116 kB]
Get:2 http://deb.debian.org/debian-security bullseye-security InRelease [48.4 kB]
Get:3 http://deb.debian.org/debian bullseye-updates InRelease [44.1 kB]
Get:4 http://deb.debian.org/debian bullseye-backports InRelease [49.0 kB]
Get:5 https://dl.yarnpkg.com/debian stable InRelease [17.1 kB]
Get:6 http://deb.debian.org/debian buster InRelease [122 kB]
Get:7 https://deb.nodesource.com/node_18.x bullseye InRelease [4586 B]
Get:8 http://deb.debian.org/debian bullseye/main amd64 Packages [8183 kB]
Get:9 http://deb.debian.org/debian-security bullseye-security/main amd64 Packages [243 kB]
Get:10 http://deb.debian.org/debian bullseye-updates/main amd64 Packages [14.6 kB]
Get:11 http://deb.debian.org/debian bullseye-backports/main amd64 Packages [420 kB]
Get:12 https://dl.yarnpkg.com/debian stable/main amd64 Packages [10.9 kB]
Get:13 https://dl.yarnpkg.com/debian stable/main all Packages [10.9 kB]
Get:14 http://deb.debian.org/debian buster/main amd64 Packages [7909 kB]
Get:15 https://deb.nodesource.com/node_18.x bullseye/main amd64 Packages [776 B]
Fetched 17.2 MB in 2s (7519 kB/s)
Reading package lists...

Reading package lists...
Building dependency tree...
Reading state information...
The following additional packages will be installed:
  libpopt0
Suggested packages:
  openssh-client openssh-server
The following NEW packages will be installed:
  libpopt0 rsync
0 upgraded, 2 newly installed, 0 to remove and 56 not upgraded.
Need to get 446 kB of archives.
After this operation, 987 kB of additional disk space will be used.
Do you want to continue? [Y/n] Abort.

Progress!

Borrowing from my past experience with Docker, I've used exactly this line … let's find out if it works.

Or rather this — I'm no Linux/Debian expert at all. (A quick web search appears to confirm this: https://serverfault.com/questions/227190/how-do-i-ask-apt-get-to-skip-an....)

The last submitted patch, 10: rsync4.patch, failed testing. View results →

That worked! Only failed due to 📌 Since #3361983 was committed to Drupal core, psr/http-message needed to be explicitly required for build tests Needs work , which was just fixed.

Re-testing…

Ok the problem now is that \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApi fails but only if it is using the rsync file syncer not the php version. The build test have not changed otherwise.

From the error https://www.drupal.org/pift-ci-job/2673917 →

Error response: The website encountered an unexpected error. Please try again later.Drupal\package_manager\Exception\StageException: The command "'/usr/local/bin/composer' '--working-dir=/tmp/.package_managerf5b6ab4c-f179-4dd0-b9d6-fbd7e445b4dc/mVjIW8ONB09LB02ZopNVsGgoIutcOz1OVu1jK4UqtBA' 'update' '--with-all-dependencies' 'drupal/core-composer-scaffold:9.8.1' 'drupal/core-project-message:9.8.1' 'drupal/core-recommended:9.8.1' 'drupal/core-dev:9.8.1'" failed.

Exit Code: 1(General error)

Working directory: /tmp/build_workspace_edb7b6704afb341ebc404be56ec78840KxpJYp/project/web

Output:
================
phpstan/extension-installer: Extensions installed
> mglaman/phpstan-drupal: installed
> phpstan/phpstan-phpunit: installed
Scaffolding files for drupal/core:
- Copy [project-root]/.editorconfig from assets/scaffold/files/editorconfig
- Copy [project-root]/.gitattributes from assets/scaffold/files/gitattributes

Error Output:
================
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 5 updates, 0 removals
- Upgrading drupal/core (9.8.0 => 9.8.1)
- Upgrading drupal/core-composer-scaffold (9.8.0 => 9.8.1)
- Upgrading drupal/core-dev (9.8.0 => 9.8.1)
- Upgrading drupal/core-project-message (9.8.0 => 9.8.1)
- Upgrading drupal/core-recommended (9.8.0 => 9.8.1)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 5 updates, 0 removals
0 [>---------------------------] 0 [->--------------------------]
- Upgrading drupal/core-composer-scaffold (9.8.0 => 9.8.1): Mirroring from /tmp/build_workspace_edb7b6704afb341ebc404be56ec78840KxpJYp/composer/Plugin/Scaffold
- Upgrading drupal/core-project-message (9.8.0 => 9.8.1): Mirroring from /tmp/build_workspace_edb7b6704afb341ebc404be56ec78840KxpJYp/composer/Plugin/ProjectMessage
- Upgrading drupal/core (9.8.0 => 9.8.1): Mirroring from /tmp/build_workspace_edb7b6704afb341ebc404be56ec78840KxpJYp/core
- Upgrading drupal/core-dev (9.8.0 => 9.8.1)
- Upgrading drupal/core-recommended (9.8.0 => 9.8.1)
0/1 [>---------------------------] 0%
1/1 [============================] 100%
Generating autoload files

In Filesystem.php line 284:

Could not delete /tmp/.package_managerf5b6ab4c-f179-4dd0-b9d6-fbd7e445b4dc/
mVjIW8ONB09LB02ZopNVsGgoIutcOz1OVu1jK4UqtBA/web/sites/default/default.servi
ces.yml:

This looks like errors doing the composer update in the stage directory. From the message output it looks like it has already done the composer updates and then it fails on trying to delete default.services.yml

My guess is this has something to do with this being a scaffold file that is specified in core/composer.json under drupal-scaffold > file-mapping. Maybe a permission issue trying to delete the old file before the scaffold moves the new one in place. If you look in \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::setUpstreamCoreVersion we specifically alter the core/assets/scaffold/files/* version file with This is part of Drupal $version so we can check later in \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertUpdateSuccessful to make sure it was copied over. If I comment these parts out the the test passes.

\Drupal\Composer\Plugin\Scaffold\Operations\ReplaceOp::process does try to remove the destination file first so this might be where the error is coming from.

Not sure why this would happen with the rsync file copier and not the php file copier. But since this is failing on running the composer update in the staged copy and the file copier is not involved in that step then my guess is it is because the "create" stage where the files are copied from active to stage by the file copier happens differently with the php copier vs the rsync copier.

One guess is the permissions are different in stage after being copied with rsync copier vs the php file copier. I would the permission would be the same but it might be the one leaves the permissions wrong.

I think there are 2 possibilities if this really is where the problem is coming from

1. The rsync copier is actually copying the file with the correct permission and the php file copier has always been doing the wrong permissions. Then our tests were only passing before because of this
2. Rsync copier is messing up the permissions somehow and if it was corrected our tests would pass

My guess is it is number 1 but sure

Unassigning myself in case someone wants to debug this more before I look at this again tomorrow.

Yep you were right i just confirmed that it does fail in \Drupal\Composer\Plugin\Scaffold\Operations\ReplaceOp::process on
$fs->remove($destination_path)

Woah! 😱

@omkar.podey thanks for researching that.

I think one way to test this

1. Change \Drupal\automatic_updates_test_api\ApiController to stop after copying the code base, don't run the composer update in the stage, don't apply, don't destroy
2. Add \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::$destroyBuild as False
3. Change \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApi to use @testWith for the file copier.
4. Run testApi(), this should leave 2 copies of the codebase staged on in your tmp directory. One used php copier and one using resync copier. Would have to not which order the test cases happened to know the different

   Though you could something like add this make easier

   $this->createTestProject('RecommendedProject', $file_copier);
       file_put_contents($this->getWebRoot() . "/copier-$file_copier.txt", "Used $file_copier");
   

5. Inspected permission of sites/default/default.services.yml and directories above it. Are they different between the 2 copiers?

If the permissions are different this might be the problem

1. I pushed up the branch 3362143-use-rsync_debug that implements the idea #18

   As far as I can tell the permissions and ownership of the files seem the same

   Php copier

   ➜ default ls -al
   total 168
   dr-xr--r-- 6 ted.bowman wheel 192 May 24 09:30 .
   drwxr-xr-x 8 ted.bowman wheel 256 May 24 09:36 ..
   -rw-r--r-- 1 ted.bowman wheel 9101 May 24 09:30 default.services.yml
   -rw-r--r-- 1 ted.bowman wheel 34736 May 24 09:30 default.settings.php
   drwxrwxr-x 6 ted.bowman wheel 192 May 24 09:32 files
   -r-xr--r-- 1 ted.bowman wheel 35823 May 24 09:31 settings.php
   ➜ default cd ..
   ➜ sites ls -al
   total 56
   drwxr-xr-x 8 ted.bowman wheel 256 May 24 09:36 .
   drwxr-xr-x 22 ted.bowman wheel 704 May 24 09:36 ..
   -rw-r--r--@ 1 ted.bowman wheel 6148 May 24 09:36 .DS_Store
   -rw-r--r-- 1 ted.bowman wheel 515 May 24 09:30 README.txt
   dr-xr--r-- 6 ted.bowman wheel 192 May 24 09:30 default
   -rw-r--r-- 1 ted.bowman wheel 310 May 24 09:30 development.services.yml
   -rw-r--r-- 1 ted.bowman wheel 5712 May 24 09:30 example.settings.local.php
   -rw-r--r-- 1 ted.bowman wheel 2353 May 24 09:30 example.sites.php
   ➜ sites pwd
   /private/tmp/build_workspace_0ddf880575312372786ce46a74c83133vPQa0y/project/web/sites
   ➜ sites ls -al ..
   total 136
   drwxr-xr-x 22 ted.bowman wheel 704 May 24 09:36 .
   drwxr-xr-x 9 ted.bowman wheel 288 May 24 09:36 ..
   -rw-r--r--@ 1 ted.bowman wheel 6148 May 24 09:36 .DS_Store
   -rw-r--r-- 1 ted.bowman wheel 1025 May 24 09:30 .csslintrc
   -rw-r--r-- 1 ted.bowman wheel 151 May 24 09:30 .eslintignore
   -rw-r--r-- 1 ted.bowman wheel 41 May 24 09:30 .eslintrc.json
   -rw-r--r-- 1 ted.bowman wheel 2467 May 24 09:30 .ht.router.php
   -rw-r--r-- 1 ted.bowman wheel 8024 May 24 09:30 .htaccess
   -rw-r--r-- 1 ted.bowman wheel 94 May 24 09:30 INSTALL.txt
   -rw-r--r-- 1 ted.bowman wheel 3205 May 24 09:30 README.md
   -rw-r--r-- 1 ted.bowman wheel 315 May 24 09:30 autoload.php
   -rw-r--r-- 1 ted.bowman wheel 8 May 24 09:31 copier-php.txt
   drwxr-xr-x 55 ted.bowman wheel 1760 May 24 09:30 core
   -rw-r--r-- 1 ted.bowman wheel 1495 May 24 09:30 example.gitignore
   -rw-r--r-- 1 ted.bowman wheel 549 May 24 09:30 index.php
   drwxr-xr-x 4 ted.bowman wheel 128 May 24 09:30 modules
   drwxr-xr-x 3 ted.bowman wheel 96 May 24 09:30 profiles
   -rw-r--r-- 1 ted.bowman wheel 1706 May 24 09:30 robots.txt
   drwxr-xr-x 8 ted.bowman wheel 256 May 24 09:36 sites
   drwxr-xr-x 3 ted.bowman wheel 96 May 24 09:30 themes
   -rw-r--r-- 1 ted.bowman wheel 804 May 24 09:30 update.php
   -rw-r--r-- 1 ted.bowman wheel 4039 May 24 09:30 web.config
   ➜ sites pwd
   /private/tmp/build_workspace_0ddf880575312372786ce46a74c83133vPQa0y/project/web/sites

   see the copier-php.txt make sure I didn't use the wrong directory

   rsync copier

   default ls -al
   total 168
   dr-xr--r-- 6 ted.bowman wheel 192 May 24 09:32 .
   drwxr-xr-x 7 ted.bowman wheel 224 May 24 09:32 ..
   -rw-r--r-- 1 ted.bowman wheel 9101 May 24 09:32 default.services.yml
   -rw-r--r-- 1 ted.bowman wheel 34736 May 24 09:32 default.settings.php
   drwxrwxr-x 6 ted.bowman wheel 192 May 24 09:33 files
   -r-xr--r-- 1 ted.bowman wheel 35825 May 24 09:32 settings.php
   ➜ default cd .
   ➜ default cd ..
   ➜ sites ls -al
   total 56
   drwxr-xr-x 8 ted.bowman wheel 256 May 24 09:34 .
   drwxr-xr-x 22 ted.bowman wheel 704 May 24 09:34 ..
   -rw-r--r--@ 1 ted.bowman wheel 6148 May 24 09:34 .DS_Store
   -rw-r--r-- 1 ted.bowman wheel 515 May 24 09:32 README.txt
   dr-xr--r-- 6 ted.bowman wheel 192 May 24 09:32 default
   -rw-r--r-- 1 ted.bowman wheel 310 May 24 09:32 development.services.yml
   -rw-r--r-- 1 ted.bowman wheel 5712 May 24 09:32 example.settings.local.php
   -rw-r--r-- 1 ted.bowman wheel 2353 May 24 09:32 example.sites.php
   ➜ sites pwd
   /private/tmp/build_workspace_74b987b4b4c6ac8e7d589ab6ddcace65dZmtDj/project/web/sites
   ➜ sites ls -al ..
   total 136
   drwxr-xr-x 22 ted.bowman wheel 704 May 24 09:34 .
   drwxr-xr-x 9 ted.bowman wheel 288 May 24 09:33 ..
   -rw-r--r--@ 1 ted.bowman wheel 6148 May 24 09:34 .DS_Store
   -rw-r--r-- 1 ted.bowman wheel 1025 May 24 09:32 .csslintrc
   -rw-r--r-- 1 ted.bowman wheel 151 May 24 09:32 .eslintignore
   -rw-r--r-- 1 ted.bowman wheel 41 May 24 09:32 .eslintrc.json
   -rw-r--r-- 1 ted.bowman wheel 2467 May 24 09:32 .ht.router.php
   -rw-r--r-- 1 ted.bowman wheel 8024 May 24 09:32 .htaccess
   -rw-r--r-- 1 ted.bowman wheel 94 May 24 09:32 INSTALL.txt
   -rw-r--r-- 1 ted.bowman wheel 3205 May 24 09:32 README.md
   -rw-r--r-- 1 ted.bowman wheel 315 May 24 09:32 autoload.php
   -rw-r--r-- 1 ted.bowman wheel 10 May 24 09:33 copier-rsync.txt
   drwxr-xr-x 55 ted.bowman wheel 1760 May 24 09:32 core
   -rw-r--r-- 1 ted.bowman wheel 1495 May 24 09:32 example.gitignore
   -rw-r--r-- 1 ted.bowman wheel 549 May 24 09:32 index.php
   drwxr-xr-x 4 ted.bowman wheel 128 May 24 09:32 modules
   drwxr-xr-x 3 ted.bowman wheel 96 May 24 09:32 profiles
   -rw-r--r-- 1 ted.bowman wheel 1706 May 24 09:32 robots.txt
   drwxr-xr-x 8 ted.bowman wheel 256 May 24 09:34 sites
   drwxr-xr-x 3 ted.bowman wheel 96 May 24 09:32 themes
   -rw-r--r-- 1 ted.bowman wheel 804 May 24 09:32 update.php
   -rw-r--r-- 1 ted.bowman wheel 4039 May 24 09:32 web.config
   ➜ sites cd sites

   see the copier-rsync.txt to see I got the correct directory

   🤔so I am not sure what is going on here. Someone should look at listing and see if I am missing something.

   Also could be good if someone else checked 3362143-use-rsync_debug and see if they get the same results

2. Locally I also changed the @testWith for \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApi to run the rsync copier test case first. Just to make sure it wasn't the copier used and the second test case would always fail no matter which copier. Rsync failed whether it was first or second

   This seem very unlikely to be the cause but wanted to rule it out

1. Ok did some testing without automatic updates involved.

   I wanted to manually test the composer operation without Automatic Updates or Composer stager involved

   I don't think default.services.yml has changed between 10.0.0 and 10.0.9 so I couldn't test that. But default.settings.yml has changed between 10.0.6 and 10.0.9 so I could test this and the same permissions issues applied.

   Here are commands I ran

     composer create-project drupal/recommended-project:10.0.6 scaff-test  
     cd scaff-test/web 
      php ./core/scripts/drupal quick-start standard  // To properly install drupal which sets file permissions
     // check the status report, and ctrl c to cancel
      cd ..  
     composer require drupal/core-recommended:10.0.9 drupal/core-composer-scaffold:10.0.9 drupal/core-project-message:10.0.9 -W
     

   This failed with a very similar error.

       - Upgrading guzzlehttp/psr7 (2.4.4 => 2.4.5): Extracting archive
       - Upgrading guzzlehttp/promises (1.5.2 => 1.5.3): Extracting archive
       - Upgrading guzzlehttp/guzzle (7.5.0 => 7.5.3): Extracting archive
       - Upgrading drupal/core (10.0.6 => 10.0.9): Extracting archive
       - Upgrading drupal/core-recommended (10.0.6 => 10.0.9)
     Generating autoload files
     36 packages you are using are looking for funding.
     Use the `composer fund` command to find out more!
     Scaffolding files for drupal/core:
       - Copy [web-root]/.htaccess from assets/scaffold/files/htaccess
     
     In Filesystem.php line 284:
                                                                                                    
       Could not delete /Users/ted.bowman/sites/scaff-test/web/sites/default/default.settings.php:  
     

   Even same line number Filesystem.php line 284:

   Turns out you could argue the scaffold plugin doesn't handle permission correctly because you are have to do manual steps.

   see https://www.drupal.org/forum/support/upgrading-drupal/2022-06-25/compose... →
   https://drupal.stackexchange.com/questions/290296/composer-require-fails...

   I will look to see if there is core issue. Or maybe we have to do some permissions changing ourselves

2. This still leaves the question of why we didn't encounter this with the PHP file copier

Turns out you could argue the scaffold plugin doesn't handle permission correctly because you are have to do manual steps.

Is it really the scaffold plugin, or is it just file system permissions?

Those files have these permissions (this is from a git clone of Drupal core):

$ ls -al sites/default/default*
-rw-r--r--  1 wim.leers  staff   9069 May 12 16:05 sites/default/default.services.yml
-rw-r--r--  1 wim.leers  staff  34704 May 12 16:15 sites/default/default.settings.php

which is the same as any other file in Drupal core:

$ ls -al core/*.txt
-rw-r--r--  1 wim.leers  staff    399 Apr 23 13:18 core/CHANGELOG.txt
-rw-r--r--  1 wim.leers  staff   2029 Apr 23 13:18 core/COPYRIGHT.txt
-rw-r--r--  1 wim.leers  staff   1717 Apr 23 13:18 core/INSTALL.mysql.txt
-rw-r--r--  1 wim.leers  staff   1874 Apr 23 13:18 core/INSTALL.pgsql.txt
-rw-r--r--  1 wim.leers  staff   1415 Apr 23 13:18 core/INSTALL.sqlite.txt
-rw-r--r--  1 wim.leers  staff  19868 May 12 16:05 core/INSTALL.txt
-rw-r--r--  1 wim.leers  staff  18002 Apr 23 13:18 core/LICENSE.txt
-rw-r--r--  1 wim.leers  staff  16070 May 12 16:15 core/MAINTAINERS.txt
-rw-r--r--  1 wim.leers  staff   2455 Apr 23 13:18 core/UPDATE.txt
-rw-r--r--  1 wim.leers  staff   4373 Apr 23 13:18 core/USAGE.txt

… so whichever user is the owner is the only one able to modify these. (-rw-r--r-- aka sudo chmod 644, which also exists in core's commit-code-check.sh.)

So I'm now wondering if we're executing composer as a different user than the owner. Could you check that? 🙏

I think that is what arguably \Drupal\Composer\Plugin\Scaffold needs to check, because I find zero matches for:

… in that namespace 😬

Found the relevant core issue: 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work .

Turns out there is core issue for this 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work

that has a suggested work around 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work

As one work around that issue points the scaffold docs https://www.drupal.org/docs/develop/using-composer/using-drupals-compose... →

That would allow excluding certain scaffold files that you no longer need updates for by updating the project's composer.json

I think could think of 2 solutions

1. Require project's composer.json to set extra->drupal-scaffold->file-mapping to exclude default.* scaffold fills which are not necessary for the site to function

   If it is choice between reliably providing security updates and having these files updated then security updates is probably more important.

   In the future we could even provide a "prepare my site" form that would set this automatically through package manager itself.

2. We could try to set the permission automatically and then re-harden them before and after composer operations

I am in favor of #1 because it seems less error prone

But the core issue does not seem to mention the last paragraph of #21 as the root cause. But if that is a correct extrapolation from me … then we would be able to write a validator for it! 😊🥳

So fingers crossed 😄🤞

Note that if do all the steps in #20 except

php ./core/scripts/drupal quick-start standard

I don't get the error. I think this because installing drupal changes these file permissions.

re #24

We do have \Drupal\automatic_updates\Validator\ScaffoldFilePermissionsValidator but maybe we have bug in there

Maybe this belongs in package_manager. On the other hand if you are using Project Browser only you probably would not run into this because it cannot update core and I think the core scaffold will only move these files if core has been updated

#23: I offered a third solution in #24, but I think I need to clarify it:

1. If the user who executed the original composer commands is not the same user as PM (Package Manager)/AU (Automatic Updates) will execute composer as, this problem will occur.

   In other words: this is "just" a manifestation of what 📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed was all about: properly secured sites have their executable code owned by a user that is not the site owner.

   So the most secure solution is to (i.e. run whoami from a validator in PM and check if it does NOT match the owner of sites/default/default.settings.php), inform the user that a cron command be set up that invokes the Symony Console (currently still Drush — see 📌 Add Symfony Console command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed ) command.

   This also means that PB (Project Browser) will not be able to install any project, only browsing projects. (Unless we add a queuing system that will be executed by that command whenever it is invoked via crontab.)

   PM/AU (and PB) de facto run in "reduced security" mode (which is how pretty much every WordPress site is run). When this is the case, all functionality is available through the UI.

my understanding is correct, then I think the above is the only approach that will be accepted by Drupal's security team, and hence it'd block getting PM + AU to stable.

We do have \Drupal\automatic_updates\Validator\ScaffoldFilePermissionsValidator but maybe we have bug in there

Its logic:

    // Flag messages about anything in $paths which exists, but isn't writable.
    $non_writable_files = array_filter($paths, function (string $path): bool {
      return file_exists($path) && !is_writable($path);
    });
    if ($non_writable_files) {
      // Re-key the messages in order to prevent false negative comparisons in
      // tests.
      $non_writable_files = array_map($this->t(...), array_values($non_writable_files));
      $event->addError($non_writable_files, $this->t('The following paths must be writable in order to update default site configuration files.'));
    }

👆 I think that is_writable() may just not be doing what we think it does. At least some comments on the official PHP docs say that too:

It appears that is_writable() does not check full permissions of a file to determine whether the current user can write to it. For example, with Apache running as user 'www', and a member of the group 'wheel', is_writable() returns false on a file like

-rwxrwxr-x root wheel /etc/some.file

— https://www.php.net/manual/en/function.is-writable.php#61331,

So maybe it just checks that it's writable by a user?

OTOH it works fine on my system:

$ ls -al /Users/wim.leers/core/sites/default/default.settings.php
-rw-r--r--  1 wim.leers  staff  34704 May 12 16:15 /Users/wim.leers/core/sites/default/default.settings.php
$ php -r "var_dump(is_writable('/Users/wim.leers/core/sites/default/default.settings.php'));"
bool(true)
$ sudo chown root:staff  /Users/wim.leers/core/sites/default/default.settings.php
$ ls -al /Users/wim.leers/core/sites/default/default.settings.php
-rw-r--r--  1 root  staff  34704 May 12 16:15 /Users/wim.leers/core/sites/default/default.settings.php
$ php -r "var_dump(is_writable('/Users/wim.leers/core/sites/default/default.settings.php'));"
bool(false)

🤔

Investigating the failure.

It's very peculiar that \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApi() fails tests but ::testUi doesn't. So we end up with this matrix:

In principle, they should run exactly the same composer commands in the exact same way.

This makes no sense. If the entire rsync row was ❌, it would make sense.

There must be a subtle bug (behavior difference) somewhere.

#20:

Even same line number Filesystem.php line 284:

I thought this was referring to Drupal's Filesystem. But it's composer's!

See https://github.com/composer/composer/blob/2.5.5/src/Composer/Util/Filesy....

Also rang alarm bells at #3091285-92: Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. → .

@tedbow's conclusion in #23 where he wrote is the only viable path forward I see right now.

I traced into this, and what I found was straightforward: when the stage directory is created, sites/default (the directory) has different permissions depending on whether the file syncer is rsync, or the PHP syncer.

In the active directory, the permissions are 644. That's because Drupal actively tries to write-protect the directory (it does this at install time, and in system_requirements()) for security hardening. Makes sense.

Now, if you're using the rsync file syncer, those permissions are preserved when the stage directory is created. That's because Composer Stager calls rsync with the --archive option, which implies the --perms option, which preserves permissions. Because of that, the scaffold files cannot be updated in sites/default, and the update fails due to ScaffoldFilePermissionsValidator. Makes sense.

The PHP file syncer, as it turns out, is less reliable. It delegates to Symfony's Filesystem component (specifically the \Symfony\Component\Filesystem\Filesystem::copy() method), which does try to preserve permissions on copied files...but it does not preserve permissions on directories. When creating directories, it does this:

$this->mkdir(\dirname($targetFile));

\Symfony\Component\Filesystem\Filesystem::mkdir() takes a second argument -- the permissions, which default to 777. But copy() doesn't pass that argument, so the directory is created with 777 permissions.

This causes our build tests, which use the PHP file syncer by default, to pass by coincidence. As it turns out, we do try to ensure that we're handling permissions of sites/default properly -- if you look at \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::createTestProject(), this is the last bit of the method:

    // Ensure that Drupal has write-protected the site directory.
    $this->assertDirectoryIsNotWritable($this->getWebRoot() . '/sites/default');

What we didn't realize, though, is that the PHP file syncer is disregarding those permissions, and creating the staged copy of sites/default with world-writable permissions. So this test is wrongfully, but understandably, assuming that the permissions from the active sites/default directory are reflected in the staged sites/default.

So, as part of fixing this problem, we will want to be certain that the staged sites/default directory has exactly the same permissions as the active one, in all cases.

@phenaproxima thanks for the research on #35

I think that given this we have to switch the default file_syncer.

We already have this 📌 Warn strongly if the rsync file syncer is not in use Fixed but maybe we should change that be an error if you are using the php file copier. We found this problem with sites/default but is it safe to use the php file copier if we know will reset directory permissions

phenaproxima → closed merge request !893

phenaproxima → closed merge request !892

Looks good!!!!

Quoting https://git.drupalcode.org/project/automatic_updates/-/merge_requests/84...

Of course, once we make rsync the actual default, then this won't be needed anymore! But this is operating under the assumption that we want the php file syncer to still be possible to use. Otherwise, what's the point of even having it?

I very strongly disagree with:

- file_syncer: php
+ # The rsync file syncer is currently the only stable file syncer and
+ # should only be changed for development purposes.
+ file_syncer: rsync

This makes no sense. We're assuming rsync is available on every system. We're not even warning the user. It will fail hard, without a warning.

IF we were to do this, we'd at minimum need a hook_requirements() check. And that's precisely what 📌 Warn strongly if the rsync file syncer is not in use Fixed is doing.

But really, if we're going to do this, why even have the php file syncer?!

The proper solution is to:

• have a default setting that works on all environments — that means reverting the above diff I quoted
• detect the presence of a superior option (rsync) and automatically switch to it — that's present in the MR for #3355105 but was removed by @phenaproxima at https://git.drupalcode.org/project/automatic_updates/-/merge_requests/84...

This was hastily done and is incomplete. We should do better. This is very confusing 😞

#49

have a default setting that works on all environments — that means reverting the above diff I quoted

That is currently not possible. PHP file copier does not retain correct directory permissions as explained in #35. We found this issue because the scaffold had an error when we switched to rsync which does retain correct file permissions. The fact that folder permissions get changed from a protected status to 777 is not safe or at least we should assume it is not till when investigate more.

But really, if we're going to do this, why even have the php file syncer?!

Hopefully we can get these permissions problems solved with the PHP file syncer or look into compensating on side by fixing permissions. I don't think it is worth totally removing it right now.

We're not even warning the user. It will fail hard, without a warning.

We should confirm this and make sure Composer stager itself does not have a pre-condition for this situations

Right now \PhpTuf\ComposerStager\Infrastructure\Service\Finder\ExecutableFinder::find would throw an exception in this situation probably which would error on our begin phase

This was hastily done and is incomplete.

Not sure about others, I personally don't find comments like this helpful and find them demotivating

I created this follow-up 📌 Determine if the PHP syncer can and should be the default file copier Needs work

We're not even warning the user. It will fail hard, without a warning.

Since 📌 Warn strongly if the rsync file syncer is not in use Fixed we will add an error in pre-create in `RsyncValidator`

👍

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