Issue #3257849 by tedbow: Various core quality updates

automatic_updates.info.yml

 name: 'Automatic Updates'
 type: module
-description: 'Experimental module to develop automatic updates. Currently the module provides checks for update readiness but does not yet provide update functionality.'
+description: 'Automatically updates Drupal core.'
 core_version_requirement: ^9.2
 dependencies:
   - drupal:automatic_updates_9_3_shim

automatic_updates.install

@@ -12,6 +12,7 @@ use Drupal\automatic_updates\Validation\ReadinessRequirements;
  */
 function automatic_updates_requirements($phase) {
   if ($phase === 'runtime') {
+    // Check that site is ready to perform automatic updates.
     /** @var \Drupal\automatic_updates\Validation\ReadinessRequirements $readiness_requirement */
     $readiness_requirement = \Drupal::classResolver(ReadinessRequirements::class);
     return $readiness_requirement->getRequirements();

automatic_updates.module

@@ -47,6 +47,7 @@ function automatic_updates_page_top() {
     'automatic_updates.report_update',
     'automatic_updates.module_update',
   ];
+  // @see auto_updates_module_implements_alter()
   $route_name = \Drupal::routeMatch()->getRouteName();
   if (!in_array($route_name, $skip_routes, TRUE) && function_exists('update_page_top')) {
     update_page_top();
@@ -63,8 +64,7 @@ function automatic_updates_module_implements_alter(&$implementations, $hook) {
     // Remove hook_page_top() implementation from the Update module. This '
     // implementation displays error messages about security releases. We call
     // this implementation in our own automatic_updates_page_top() except on our
-    // own routes to avoid stale messages about the security releases after an
-    // update.
+    // own routes to avoid these messages while an update is in progress.
     unset($implementations['update']);
   }
 }
@@ -114,7 +114,9 @@ function automatic_updates_modules_uninstalled() {
  */
 function automatic_updates_form_update_manager_update_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   // Remove current message that core updates are not supported with a link to
-  // use this modules form.
+  // use this module's form. The local task to 'update_manager_update_form' is
+  // replaced by our own from but this original form would still accessible via
+  // by its original URL.
   if (isset($form['manual_updates']['#rows']['drupal']['data']['title'])) {
     $current_route = \Drupal::routeMatch()->getRouteName();
     if ($current_route === 'update.module_update') {
@@ -141,6 +143,8 @@ function automatic_updates_form_update_settings_alter(array &$form, FormStateInt
   $drupal_project = $recommender->getProjectInfo();
   $version = ExtensionVersion::createFromVersionString($drupal_project['existing_version']);
   $current_minor = $version->getMajorVersion() . '.' . $version->getMinorVersion();
+  // @todo In https://www.drupal.org/node/2998285 use the update XML to
+  //   determine when the installed of core will become unsupported.
   $supported_until_version = $version->getMajorVersion() . '.'
       . ((int) $version->getMinorVersion() + ProjectSecurityData::CORE_MINORS_WITH_SECURITY_COVERAGE)
       . '.0';

automatic_updates.routing.yml

@@ -13,7 +13,7 @@ automatic_updates.confirmation_page:
   requirements:
     _permission: 'administer software updates'
     _access_update_manager: 'TRUE'
-# Links to our updater form appear in two different sets of local tasks. To ensure the breadcrumbs and paths are
+# Links to our updater form appear in three different sets of local tasks. To ensure the breadcrumbs and paths are
 # consistent with the other local tasks in each set, we need two separate routes to the same form.
 automatic_updates.report_update:
   path: '/admin/reports/updates/automatic-update'

package_manager/package_manager.info.yml

 name: 'Package Manager'
 type: module
-description: 'API module providing functionality for staging package updates.'
+description: 'API module providing functionality for staging package installs and updates with Composer.'
 core_version_requirement: ^9

package_manager/src/ComposerUtility.php

@@ -48,7 +48,7 @@ class ComposerUtility {
   }
 
   /**
-   * Creates a utility object using the files in a given directory.
+   * Creates an instance of this class using the files in a given directory.
    *
    * @param string $dir
    *   The directory that contains composer.json and composer.lock.
@@ -111,7 +111,8 @@ class ComposerUtility {
    * @return string[]
    *   The names of the required core packages.
    *
-   * @todo Make this return a keyed array of packages, not just names.
+   * @todo Make this return a keyed array of packages, not just names in
+   *   https://www.drupal.org/i/3258059.
    */
   public function getCorePackageNames(): array {
     $core_packages = array_intersect(
@@ -135,7 +136,8 @@ class ComposerUtility {
    * @return string[]
    *   The names of the core packages in the dev requirements.
    *
-   * @todo Make this return a keyed array of packages, not just names.
+   * @todo Make this return a keyed array of packages, not just names in
+   *   https://www.drupal.org/i/3258059.
    */
   public function getCoreDevPackageNames(): array {
     $dev_packages = $this->composer->getPackage()->getDevRequires();

package_manager/src/Event/PostDestroyEvent.php

@@ -4,6 +4,11 @@ namespace Drupal\package_manager\Event;
 
 /**
  * Event fired after the staging area is destroyed.
+ *
+ * If the stage is being force destroyed, ::getStage() may return an object of a
+ * different class than the one that originally created the staging area.
+ *
+ * @see \Drupal\package_manager\Stage::destroy()
  */
 class PostDestroyEvent extends StageEvent {
 }

package_manager/src/Event/PostRequireEvent.php

@@ -3,7 +3,7 @@
 namespace Drupal\package_manager\Event;
 
 /**
- * Event fired after packages are added to the staging area.
+ * Event fired after packages are updated to the staging area.
  */
 class PostRequireEvent extends StageEvent {
 }

package_manager/src/Event/PreDestroyEvent.php

@@ -4,6 +4,11 @@ namespace Drupal\package_manager\Event;
 
 /**
  * Event fired before the staging area is destroyed.
+ *
+ * If the stage is being force destroyed, ::getStage() may return an object of a
+ * different class than the one that originally created the staging area.
+ *
+ * @see \Drupal\package_manager\Stage::destroy()
  */
 class PreDestroyEvent extends PreOperationStageEvent {
 }

package_manager/src/Event/PreOperationStageEvent.php

@@ -11,7 +11,7 @@ use Drupal\package_manager\ValidationResult;
 abstract class PreOperationStageEvent extends StageEvent {
 
   /**
-   * {@inheritdoc}
+   * Adds error information to the event.
    */
   public function addError(array $messages, ?TranslatableMarkup $summary = NULL) {
     $this->results[] = ValidationResult::createError($messages, $summary);

package_manager/src/Event/PreRequireEvent.php

@@ -3,7 +3,7 @@
 namespace Drupal\package_manager\Event;
 
 /**
- * Event fired before packages are added to the staging area.
+ * Event fired before packages are updated to the staging area.
  */
 class PreRequireEvent extends PreOperationStageEvent {
 }

package_manager/src/FileSyncerFactory.php

@@ -9,7 +9,7 @@ use PhpTuf\ComposerStager\Infrastructure\FileSyncer\FileSyncerFactory as StagerF
 use Symfony\Component\Process\ExecutableFinder;
 
 /**
- * A file syncer factory which returns file syncers according to configuration.
+ * A file syncer factory which creates a file syncer according to configuration.
  */
 class FileSyncerFactory implements FileSyncerFactoryInterface {
 

package_manager/src/ProcessFactory.php

@@ -10,8 +10,6 @@ use Symfony\Component\Process\Process;
 
 /**
  * Defines a process factory which sets the COMPOSER_HOME environment variable.
- *
- * @todo Figure out how to do this in composer_stager.
  */
 final class ProcessFactory implements ProcessFactoryInterface {
 

package_manager/src/Stage.php

@@ -61,21 +61,21 @@ class Stage {
   protected $pathLocator;
 
   /**
-   * The beginner service from Composer Stager.
+   * The beginner service.
    *
    * @var \PhpTuf\ComposerStager\Domain\BeginnerInterface
    */
   protected $beginner;
 
   /**
-   * The stager service from Composer Stager.
+   * The stager service.
    *
    * @var \PhpTuf\ComposerStager\Domain\StagerInterface
    */
   protected $stager;
 
   /**
-   * The committer service from Composer Stager.
+   * The committer service.
    *
    * @var \PhpTuf\ComposerStager\Domain\CommitterInterface
    */
@@ -117,11 +117,11 @@ class Stage {
    * @param \Drupal\package_manager\PathLocator $path_locator
    *   The path locator service.
    * @param \PhpTuf\ComposerStager\Domain\BeginnerInterface $beginner
-   *   The beginner service from Composer Stager.
+   *   The beginner service.
    * @param \PhpTuf\ComposerStager\Domain\StagerInterface $stager
-   *   The stager service from Composer Stager.
+   *   The stager service.
    * @param \PhpTuf\ComposerStager\Domain\CommitterInterface $committer
-   *   The committer service from Composer Stager.
+   *   The committer service.
    * @param \Drupal\Core\File\FileSystemInterface $file_system
    *   The file system service.
    * @param \Symfony\Contracts\EventDispatcher\EventDispatcherInterface $event_dispatcher
@@ -199,6 +199,9 @@ class Stage {
    *   performing other operations on it. Calling code should store this ID for
    *   as long as the stage needs to exist.
    *
+   * @throws \Drupal\package_manager\Exception\StageException
+   *   Thrown if a staging area already exists.
+   *
    * @see ::claim()
    */
   public function create(): string {
@@ -246,6 +249,8 @@ class Stage {
     $this->dispatch(new PreRequireEvent($this));
     $dir = $this->getStageDirectory();
     $this->stager->stage($command, $dir);
+    // @todo Limit the update command to only packages in $constraints in
+    //   https://www.drupal.org/i/3257849.
     $this->stager->stage(['update', '--with-all-dependencies'], $dir);
     $this->dispatch(new PostRequireEvent($this));
   }
@@ -285,6 +290,9 @@ class Stage {
     // Delete all directories in parent staging directory.
     $parent_stage_dir = static::getStagingRoot();
     if (is_dir($parent_stage_dir)) {
+      // @todo Ensure that even if attempting to delete this directory throws an
+      //   exception the stage is still marked as available in
+      //   https://www.drupal.org/i/3258048.
       $this->fileSystem->deleteRecursive($parent_stage_dir, function (string $path): void {
         $this->fileSystem->chmod($path, 0777);
       });
@@ -324,6 +332,7 @@ class Stage {
       }
     }
     catch (\Throwable $error) {
+      // @todo Simplify exception handling in https://www.drupal.org/i/3258056.
       // If we are not going to be able to create the staging area, mark it as
       // available.
       // @see ::create()
@@ -407,12 +416,16 @@ class Stage {
   }
 
   /**
-   * Ensures that the current user or session owns the staging area.
+   * Validates the ownership of staging area.
+   *
+   * The stage is considered under valid ownership if it was created by current
+   * user or session, using the current class.
    *
    * @throws \LogicException
    *   If ::claim() has not been previously called.
    * @throws \Drupal\package_manager\Exception\StageOwnershipException
-   *   If the current user or session does not own the staging area.
+   *   If the current user or session does not own the staging area, or it was
+   *   created by a different class.
    */
   final protected function checkOwnership(): void {
     if (empty($this->lock)) {

package_manager/src/Validator/ComposerExecutableValidator.php

@@ -12,7 +12,7 @@ use PhpTuf\ComposerStager\Domain\Process\Runner\ComposerRunnerInterface;
 use PhpTuf\ComposerStager\Exception\ExceptionInterface;
 
 /**
- * Validates that the Composer executable can be found in the correct version.
+ * Validates the Composer executable is the correct version.
  */
 class ComposerExecutableValidator implements PreOperationStageValidatorInterface, OutputCallbackInterface {
 

package_manager/src/Validator/DiskSpaceValidator.php

@@ -11,7 +11,7 @@ use Drupal\Core\StringTranslation\TranslationInterface;
 use Drupal\package_manager\PathLocator;
 
 /**
- * Validates that there is enough free disk space to do automatic updates.
+ * Validates that there is enough free disk space to do staging operations.
  */
 class DiskSpaceValidator implements PreOperationStageValidatorInterface {
 
@@ -103,7 +103,8 @@ class DiskSpaceValidator implements PreOperationStageValidatorInterface {
     $vendor_path = $this->pathLocator->getVendorDirectory();
     $messages = [];
 
-    // @todo Make this configurable.
+    // @todo Make this configurable or set to a different value in
+    //   https://www.drupal.org/i/3166416.
     $minimum_mb = 1024;
     $minimum_bytes = Bytes::toNumber($minimum_mb . 'M');
 

package_manager/src/Validator/LockFileValidator.php

@@ -109,7 +109,7 @@ class LockFileValidator implements PreOperationStageValidatorInterface {
     }
 
     // If we have both hashes, ensure they match.
-    if ($hash && $stored_hash && hash_equals($stored_hash, $hash) == FALSE) {
+    if ($hash && $stored_hash && !hash_equals($stored_hash, $hash)) {
       $error = $this->t('Stored lock file hash does not match the active lock file.');
     }
 

src/Event/ReadinessCheckEvent.php

@@ -53,7 +53,7 @@ class ReadinessCheckEvent extends PreOperationStageEvent {
   }
 
   /**
-   * {@inheritdoc}
+   * Adds warning information to the event.
    */
   public function addWarning(array $messages, ?TranslatableMarkup $summary = NULL) {
     $this->results[] = ValidationResult::createWarning($messages, $summary);