Problem/Motivation

drupal_check_module() is procedural code that relies on a number of external services. We should convert this to a service to make it more testable and maintainable.

Steps to reproduce

Proposed resolution

Deprecate drupal_check_module() and replace with \Drupal\Core\Extension\RequirementsChecker::checkModuleRequirements()

Remaining tasks

User interface changes

API changes

drupal_check_module() is deprecated and replaced with \Drupal\Core\Extension\RequirementsChecker::checkModuleRequirements()

Data model changes

Release notes snippet

Issue fork drupal-3409879

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

kim.pepper created an issue. See original summary.

kim.pepper opened merge request !5887

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Searched for drupal_check_module and the 1 instance has been replaced. CR is there. Think this is good.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

I'm not sure how useful this is as an API - what about moving the logic to a protected method on the form itself with no replacement? Does any contrib module call this function?

andypost’s picture

andypost
he/him
Primary language Russian
commented

4 usages in contrib http://codcontrib.hank.vps-private.net/search?text=drupal_check_module&f...

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

There is also very similar/duplicated code for checking run-time requirements in SystemManager::checkRequirements() https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

The page attachments examples from http://codcontrib.hank.vps-private.net/search?text=drupal_check_module&f... look like a bad idea, those modules could directly check their own requirements rather than jump through hoops. mmu is a reasonable example though because it's re-implementing the modules page, but then that's only one or two usages.

The ::getMaxSeverity() method on SystemManager looks identical to the one added here, so we could maybe call that from the form, or move the other helpers into SystemManager? But we definitely shouldn't duplicate that method, so marking needs work.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Hmm. System manager looks like it deals runtime requirements but also with blocks of Menu Links??

Maybe we should expand this to move the runtime requirements checks from SystemManager to the new RequirementsChecker instead?

\Drupal\system\SystemManager::listRequirements() actually calls drupal_load_updates() which depends on ModuleExtensionList as well.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Checking install and runtime requirements seem to be very different. Perhaps we should just move getMaxSeverity() to a utility class instead.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review

Back to needs review to see if you like this approach.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work
Related issues: +#2909472: Add value objects to represent the return of hook_requirements

Left a review

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

I've made changes over at #2909472: Add value objects to represent the return of hook_requirements

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.