Skip to content
Snippets Groups Projects

Issue #3437409 ForbidCoreChangesValidator.php

Merged Ted Bowman requested to merge issue/automatic_updates-3437409:3437409-dont-allow-core into 3.0.x
2 unresolved threads

Closes #3437409

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Adam G-H
  • Adam G-H
  • 92 * it ensures that the 'drupal/core' is included in the list if present.
    93 *
    94 * @param string $composer_root
    95 * The path to the composer root.
    96 *
    97 * @return \Drupal\package_manager\InstalledPackagesList
    98 * The installed core packages.
    99 */
    100 private function getInstalledCorePackages(string $composer_root): InstalledPackagesList {
    101 $installed_package_list = $this->composerInspector->getInstalledPackagesList($composer_root);
    102 $core_packages = $installed_package_list->getCorePackages();
    103 if (isset($installed_package_list['drupal/core']) && !isset($core_packages['drupal/core'])) {
    104 $core_packages = new InstalledPackagesList(array_merge($core_packages->getArrayCopy(), ['drupal/core' => $installed_package_list['drupal/core']]));
    105 }
    106 return $core_packages;
    107 }
    • Comment on lines +100 to +107

      I don't understand why we need this. drupal/core or drupal/core-recommended will be in here somewhere, right?

      IMHO this should be moved to a new method of InstalledPackagesList, called hasDrupalCore(). It would return TRUE if either drupal/core or drupal/core-recommended were there.

      public function hasDrupalCore(): bool {
        return isset($this['drupal/core']) || isset($this['drupal/core-recommended']);
      }

      Or, we could add a new bool param to getCorePackages() which doesn't remove drupal/core if drupal/core-recommended is there. (TBH I kind of like this idea better.)

      Edited by Adam G-H
    • Author Maintainer

      I would rather not make any changes to the main module in this issue. It is currently needed because \Drupal\package_manager\ComposerInspector::getInstalledPackagesList() because drupal/core could removed from the packages list.

      I think this validator should just throw an error any packages that we consider "core", \Drupal\package_manager\InstalledPackagesList::getCorePackageList are changed.

    • Please register or sign in to reply
  • 44 * @param \Drupal\package_manager\Event\StatusCheckEvent|\Drupal\package_manager\Event\PreApplyEvent $event
    45 * The event object.
    46 */
    47 public function validateStagedCorePackages(StatusCheckEvent|PreApplyEvent $event): void {
    48 $stage = $event->stage;
    49 // We only want to do this check if the stage belongs to Automatic Updates
    50 // Extensions.
    51 if ($stage->getType() !== 'automatic_updates_extensions:attended' || !$stage->stageDirectoryExists()) {
    52 return;
    53 }
    54 $active_core_packages = $this->getInstalledCorePackages($this->pathLocator->getProjectRoot());
    55 $stage_core_packages = $this->getInstalledCorePackages($stage->getStageDirectory());
    56
    57 $new_packages = $stage_core_packages->getPackagesNotIn($active_core_packages);
    58 $removed_packages = $active_core_packages->getPackagesNotIn($stage_core_packages);
    59 $changed_packages = $active_core_packages->getPackagesWithDifferentVersionsIn($stage_core_packages);
    • Comment on lines +57 to +59

      This is logic we have in several places. I think we might want to create an InstalledPackagesComparer (in another issue) which can do these kinds of computations.

    • Author Maintainer

      sure, I am not sure of all the places. I am not too worried a little duplication between the main module and this module. The sub-module will never exist by itself in core.

    • Please register or sign in to reply
  • Adam G-H
  • Adam G-H
  • Adam G-H
  • Adam G-H
  • Ted Bowman added 1 commit

    added 1 commit

    Compare with previous version

  • Ted Bowman added 3 commits

    added 3 commits

    Compare with previous version

  • Ted Bowman added 1 commit

    added 1 commit

    • afca55cb - stage requested update in test

    Compare with previous version

  • Ted Bowman added 3 commits

    added 3 commits

    Compare with previous version

  • merged

  • Please register or sign in to reply
    Loading