In place updates (from quasi-patch zip)

Not done a full review, but in general the 'get a zip, extract, copy the individual files over' seems quite vindicated by the patch, really doesn't look too bad so far.

+++ b/src/Services/Update.php
@@ -0,0 +1,301 @@
+  protected function getFilesList($directory) {

For deletions, does this need to use a manifest?

Only nitpicks, general approach looks good.

With the manifest do we definitely want this only for deletions, or for the included files too would be my only proper question.

1. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,385 @@
   +   * InPlaceUpdate constructor.
   

   I think this should be 'Constructs an InPlaceUpdate'?

2. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,385 @@
   +    $sink = $this->fileSystem->realpath($this->fileSystem->getDestinationFilename("temporary://$project_name.zip", FileSystemInterface::EXISTS_RENAME));
   

   Why sink for the variable name?

3. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,385 @@
   +   * Backup before an update.
   

   The comment says backup but the method says process.

4. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,385 @@
   +        return FALSE;
   

   Should this have a watchdog_exception()?

5. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,385 @@
   +        return FALSE;
   

   Same here.

6. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,385 @@
   +   * Backup before a backup.
   

   Backup before an update?

7. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,385 @@
   +    $this->backup = $this->fileSystem->createFilename('automatic_updates-backup', 'temporary://');
   +    $this->fileSystem->prepareDirectory($this->backup);
   +    $this->backup = $this->fileSystem->realpath($this->backup) . DIRECTORY_SEPARATOR;
   

   Should this just assign to $backup first then $this->backup after the realpath call?

This is looking really solid to me. Is there an issue open to figure out actually creating the zip releases?

There have been some concerns expressed that the work here is not compatible with what's happening in the Composer in Core initiative, so I wanted to drill down on how the zip files are being generated.

First, a couple of definitions.

The Composer in Core initiative is going to deliver "Composer-ready" tarball downloads. That means that if you download Drupal and untar it, you can immediately cd into your directory and run composer update or composer require, and the right thing will happen.

"Compsoer-ready" does not mean mix-and-match, though; once you start using Composer on an individual Drupal site, you cannot go back to drush pm-update and friends.

Our goal, then, is that if #autoupdates does not support Composer use as an update method initially that it at least maintain the "Composer ready" nature of the tarball downloads.

If the result of applying a quasi-patch zip file to a site always produces the same result as un-taring the equivalent tarball download on top of the same site (i.e. it is not missing any files except files that did not change), then the "Composer-ready" aspect of the site should be maintained.

If that's the case, I think that's sufficient for now. Of course, autoupdates will have to use Composer to update for sites that have used Composer to change their vendor directory, but I think it's okay if that is follow-on work, after the quasi-patch technique, if that's the desired development order here.

Wow this looks awesome!

There are a couple of things I had to figure out here, for testing. It currently uses a drupal.org link to find the quasi patch zips, and just trying to update a module I had locally (admin_toolbar) did not work. I assume because only token and bootstrap are available yet? Maybe we should update the IS with that?

Also, I had to look for a while to find out where I would get the QuickStartTestBase from, but figured out it was from this issue: #3031379: Add a new test type to do real update testing

So, code looks pretty solid. Some minor things and nits mostly from me :)

1. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,432 @@
   +  const CHECKSUM_LIST = 'checksumlist.csig';
   

   This is currently not being used for anything. Is it intended to be used later?

2. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,432 @@
   +    $url = $this->buildUrl($project_name, $this->getQuasiPatchFileName($project_name, $from_version, $to_version));
   

   This can theoretically return FALSE or throw an exception. Seems we should at least check for a FALSE value here?

3. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,432 @@
   +        '@version' => $version,
   

   $version is not defined. I think you meant $to_version ? :)

4. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,432 @@
   +      $file_path = substr($file->getRealPath(), strlen($this->getTempDirectory() . self::ARCHIVE_DIRECTORY));
   

   getRealPath can return a bool, which does not go well together with substr. Maybe check for that?

5. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,432 @@
   +        $result = $this->fileSystem->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY);
   

   The variable result is never used for anything

6. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,432 @@
   +        $this->fileSystem->copy($file->getRealPath(), $real_path, FileSystemInterface::EXISTS_REPLACE);
   

   Another one with a potential bool|string

7. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,432 @@
   +      $file_path = substr($file->getRealPath(), strlen($this->backup));
   

   Another one with a potential bool|string

8. +++ b/src/Services/InPlaceUpdate.php
   @@ -0,0 +1,432 @@
   +        $this->fileSystem->copy($file->getRealPath(), $this->getRealPath($file_path, $project_root), FileSystemInterface::EXISTS_REPLACE);
   

   bool|string again

Sorry I was not more explicit. But I was talking about $file->getRealPath():

https://www.php.net/manual/en/splfileinfo.getrealpath.php

Returns the path to the file, or FALSE if the file does not exist.

Although to be fair, that would be pretty unlikely, since we filter for $file->isFile() not long before. As I said. Very nit, sorry :)

Otherwise I feel my feedback was very well addressed, nice update in the IS as well.

Thanks, looking great!

Woohoo!