Consider moving hook_requirements() + \Drupal\automatic_updates\Validation\StatusCheckRequirements to package_manager

Created on 25 January 2023, over 1 year ago
Updated 14 February 2023, over 1 year ago

Problem/Motivation

Discovered while addressing #3323003-17: Mark FailureMarker @internal and document ComposerUtility, PathLocator and ValidationResult β†’ .

  1. \Drupal\package_manager\Event\StatusCheckEvent lives in the Package Manager module
  2. package_manager_requirements() does not dispatch it
  3. automatic_updates_requirements() does dispatch it, via \Drupal\automatic_updates\Validation\StatusCheckRequirements

β‡’ a status report is only generated if you install the Automatic Updates module, but what if you don't install Automatic Updates and only install Project Browser?

Steps to reproduce

Install Project Browser, not Automatic Updates: no status report with status checks!

Proposed resolution

  1. Move \Drupal\automatic_updates\Validation\StatusCheckRequirements to \Drupal\package_manager\Validation\StatusCheckRequirements
  2. Move the status report out of the Automatic Updates module and into the Package Manager module

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Active

Version

3.0

Component

Code

Created by

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Live updates comments and jobs are added and updated live.
  • Usability

    Makes Drupal easier to use. Preferred over UX, D7UX, etc.

Sign in to follow issues

Comments & Activities

  • Issue created by @Wim Leers
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    @Wim Leers interested in this idea but not sure.

    I think the original idea with Automatic Updates uniquely needs the error on status report is that it is very security focused. Site will depend on it for security updates either with no user action during cron or a user will update via the form.

    If a security release comes out today I don't want to find our today that can't apply that security update. We should warn the user as soon as possible. That is why if you cron update enabled \Drupal\automatic_updates\Validation\AdminStatusCheckMessages::displayAdminPageMessages will bother you on most admin pages(we did this at direction of the Drupal UX team/meeting). If you don't have cron updates enabled you will see this on status report page.

    I think for other uses of Package Manager where the security of your site is not at stake, this is not so clear.

    For example users may have Project Browser enabled locally to install modules for development. They may also leave it on in production even though that environment does not support Package Manager and therefore Project Browser.

    So for example WritableFileSystemValidator this give a status check error because it will definitely stop package manager from working. Project Browser will also run the status checks and put the errors on the their form. I believe they will also change their UI to not show the "Install" but if this is error is found.

    In the case where you have Project Browser installed but not Automatic Updates is a WritableFileSystemValidator error worthy of showing an Error on your status report page. Your site is not any danger if this is the case. You actually are probably safer if your file system is not writable(will core even warn if this is NOT the case).

    I didn't have a formed opinion when I started writing this comment but I think now if we also show these errors on the status report page if you just have package manager installed then people will see a lot of different errors on their status report page when the site is in different environments. This could also be true for Automatic Updates but I think is ok because Automatic Update will play such a critical(hopefully) security role for a lot of sites this is ok we should err on the side of showing the errors rather than leave them unaware until a critical security release comes out.

    For other modules I think they should follow the pattern of Project Browser and show the errors and warnings from status checks on their own UI's when the user is interacting with it.

    For other hypothetical modules that like AU provide critical security functionality that might even run in the background they could also opt for showing the status check errors on the status report.

    Another option would be for Package Manager to run the status checks on the status report but to put all messages under the "Warnings section" but then how does it tell the user which modules would be affected? I don't think adding a message about "Package Manager" is going to mean much to many admins since it is an API only module. Just look for modules that have Package Manager has a dependency? What about modules where it is not dependency but it does check if the module is able to offer additional functionality? I think core also has a problem where when you disable all modules that depend on a API only module it will leave that API only module on(correct?). If only Package Manager is enable and no other module that depends on it would we still show the error?

    I think this is core-post-mvp to figure out if this is problem and how we might solve it. Right now any module that depends on Package Manager and determines that if there functionality does work it should be an error on the status report page they can pretty easily add the messages to the status report page.

  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    WFM! πŸ‘

    Thank you for the thoughtful write-up πŸ˜ŠπŸ™

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
Production build 0.69.0 2024