Add value objects to represent the return of hook_requirements

I need something to do on a 24 hr Sydney to London flight. ;-(

Initial implementation for review.

+++ b/core/lib/Drupal/Core/Extension/Requirement/Requirement.php
@@ -0,0 +1,158 @@
+  const SEVERITY_INFO = -1;
+
+  const SEVERITY_OK = 0;
+
+  const SEVERITY_WARNING = 1;
+
+  const SEVERITY_ERROR = 2;

I'm wondering whether we should have one class per severity aka. RequirementInfo, RequirementOk, RequirementWarning and SeverityError? It feels like this would cause an ever better to read requirement.

Thanks for the review. I've switched to using properties and having a separate class for each severity level.

1. +++ b/core/lib/Drupal/Core/Extension/Requirement/BaseRequirement.php
   @@ -0,0 +1,209 @@
   +  const PHASE_INSTALL = 'install';
   ...
   +  const PHASE_UPDATE = 'update';
   ...
   +  const PHASE_RUNTIME = 'runtime';
   ...
   +  const SEVERITY_INFO = -1;
   ...
   +  const SEVERITY_OK = 0;
   ...
   +  const SEVERITY_WARNING = 1;
   ...
   +  const SEVERITY_ERROR = 2;
   

   Comments.

2. +++ b/core/lib/Drupal/Core/Extension/Requirement/BaseRequirement.php
   @@ -0,0 +1,209 @@
   +  public function offsetExists($offset) {
   

   inheritdoc?

3. +++ b/core/lib/Drupal/Core/Extension/Requirement/BaseRequirement.php
   @@ -0,0 +1,209 @@
   +    if (in_array($offset, [
   +      'description',
   +      'title',
   +      'value',
   +      'severity',
   +    ], TRUE)) {
   +      return TRUE;
   +    }
   +    return FALSE;
   

   Just return in_array...

4. +++ b/core/lib/Drupal/Core/Extension/Requirement/BaseRequirement.php
   @@ -0,0 +1,209 @@
   +      case 'severity':
   

   Shouldn't be able to set this if it's based on the class, but is this needed for BC?

5. +++ b/core/lib/Drupal/Core/Extension/Requirement/BaseRequirementOk.php
   @@ -0,0 +1,17 @@
   +class BaseRequirementOk extends BaseRequirement {
   

   No "Base" in class name?

1. Fixed
2. Fixed
3. Fixed
4. I removed the setter, but there might be a broader discussion on how we support BC with this as in #2908735: [meta] Add value objects to represent hook_schema
5. Fixed

After discussing with @Sam152, I added an interface for Requirement too.

+++ b/core/lib/Drupal/Core/Extension/Requirement/RequirementInterface.php
@@ -0,0 +1,107 @@
+  /**
+   * The OK severity.
+   */
+  const SEVERITY_OK = 0;
+
+  /**
+   * The info severity.
+   */
+  const SEVERITY_INFO = -1;
+
+  /**
+   * The error severity.
+   */
+  const SEVERITY_ERROR = 2;
+
+  /**
+   * The warning severity.
+   */
+  const SEVERITY_WARNING = 1;

Should we move this constant on the individual classes?

Should we move this constant on the individual classes?

Wouldn't it make more sense to allow you to compare different severity levels? In SystemManager we are doing:

<?php
public function getMaxSeverity(&$requirements) {
    $severity = static::REQUIREMENT_OK;
    foreach ($requirements as $requirement) {
      if (isset($requirement['severity'])) {
        $severity = max($severity, $requirement['severity']);
      }
    }
    return $severity;
  }
?>

Doing a comparison of classes to see which had the max severity would be cumbersome.

Wouldn't it make more sense to allow you to compare different severity levels? In SystemManager we are doing:

Maybe we could provide some nice helper functions for that?

Sounds like a plan. Here's a patch with the above and a helper.

+++ b/core/lib/Drupal/Core/Extension/Requirement/RequirementHelper.php
@@ -0,0 +1,39 @@
+    return array_reduce($requirements, [__CLASS__, 'max'], RequirementInterface::SEVERITY_OK);

Nice usage of array_reduce!

Thanks! I've inlined the max method to an anonymous function.

The previous fail was a random one related to Layout Builder.

The data model for a Requirement in this issue does not match the return values being implemented in #2505499: Explicitly support render arrays in hook_requirements(), specifically the supported value of description.

The related issue does indicate there is a need for a value object that outlines the correct data model.

Surprisingly, this patch still applies :)

KristenBackupMBP:drupal-9.3.x-dev admin$ patch -p1 < 2909472-18.patch 
patching file core/lib/Drupal/Core/Extension/Requirement/BaseRequirement.php
patching file core/lib/Drupal/Core/Extension/Requirement/RequirementError.php
patching file core/lib/Drupal/Core/Extension/Requirement/RequirementHelper.php
patching file core/lib/Drupal/Core/Extension/Requirement/RequirementInfo.php
patching file core/lib/Drupal/Core/Extension/Requirement/RequirementInterface.php
patching file core/lib/Drupal/Core/Extension/Requirement/RequirementOk.php
patching file core/lib/Drupal/Core/Extension/Requirement/RequirementWarning.php
patching file core/modules/system/src/SystemManager.php
Hunk #1 succeeded at 127 (offset -3 lines).
patching file core/tests/Drupal/Tests/Core/Extension/Requirement/RequirementHelperTest.php
patching file core/tests/Drupal/Tests/Core/Extension/Requirement/RequirementTest.php

Reviewed the code and there are a few items:

1. +++ b/core/lib/Drupal/Core/Extension/Requirement/RequirementHelper.php
   @@ -0,0 +1,26 @@
   +<?php
   +
   +
   +namespace Drupal\Core\Extension\Requirement;
   

   Extra line.

2. +++ b/core/modules/system/src/SystemManager.php
   @@ -130,8 +130,11 @@ public function listRequirements() {
   +   * @deprecated Will be removed in Drupal 9.0.0
   +   * @see \Drupal\Core\Extension\Requirement\RequirementHelper::getMaxSeverity()
   

   Deprecation version needs to change. Example;

   @deprecated in drupal:9.4.0 and is removed from drupal:10.0.0.
   

3. +++ b/core/tests/Drupal/Tests/Core/Extension/Requirement/RequirementHelperTest.php
   @@ -0,0 +1,73 @@
   +    $info = RequirementInfo::create()
   +      ->setValue("Foo");
   +    $warning = RequirementWarning::create()
   +      ->setValue("Baz");
   +    $error = RequirementError::create()
   +      ->setValue("Baz");
   +    $ok = RequirementOk::create()
   +      ->setValue("Bar");
   

   Nitpicks:
   1. Use different values for warning vs error.
   2. Use single quotes.

Also, please review feedback from #21.

+++ b/core/lib/Drupal/Core/Extension/Requirement/RequirementInterface.php
@@ -0,0 +1,107 @@
+  const SEVERITY_OK = 0;
...
+  const SEVERITY_INFO = -1;
...
+  const SEVERITY_ERROR = 2;
...
+  const SEVERITY_WARNING = 1;

Instead of constants it could be enum or even removed in favour of methods (like accessResult doing).
But this weight used only for frontend to group sort requirements - that usage is missing in replacement

We could possibly roll this into #3409372: [meta] Replace hook_requirements() with OOP classes

Converted to a single Requirement class with enums for RequirementSeverity and RequirementPhase.

Remaining tasks:

• Decide if we should just enforce required properties by using constructor args
• Ensure sorting works
• Replace usages of requirements arrays

Hiding old patches

Seems to have some test failures.

I think this issue is quite large, having to convert all of cores hook requirements to value objects as well as handle deprecations.

We should split out the smaller task of creating enums for RequirementSeverity and RequirementPhase #3410938: Create enums for RequirementSeverity and deprecate drupal_requirements_severity() constants

#3409372: [meta] Replace hook_requirements() with OOP classes is two thirds done.

If we can get this also into 11.2 then this might considerably simplify the BC layer. New hook use value objects, old hooks use arrays, we just need to merge it together in the places where core calls both the old and the new hooks?

Agreed this would be great to do in 11.2.0 so the new hooks only use the new API. Tagging as such. But this should now be postponed on #3410938: Create enums for RequirementSeverity and deprecate drupal_requirements_severity() constants, no?

#3410938: Create enums for RequirementSeverity and deprecate drupal_requirements_severity() constants is merged

Rebased on 11.x

Several failures, this definitely won't make it in for 11.2 I think.

Personally I would want to postpone this in the system_requirements conversion. That issue was green and just need to take the new enum into account.

I'll link it later when I fix that issue. #3493718: Convert system_requirements() into OOP hooks and install time class

Damn. I wish I saw your comment before I started converting system_requirements() 😓

I pushed up my work in progress in case it's useful later.

Working on this

Rebased on 11.x and moved everything over from system_requirements() to \Drupal\system\Install\Requirements\SystemRequirements::checkRequirements()

Took a first pass review.

We also want to handle when update requirements / phase and install time for modules and profiles during profile install amc module install.

Is this meant to be scoped to only runtime?

Most of the changes seem like that but some of the changes should run during update and install.

I'm also unsure why we're not getting failures from update and install being converted but no equivalent mapper like you put in runtime requirements during the status report.

A bunch of minor suggestions and a few substantial questions.