Move REQUIREMENT_* constants out of install.inc file

Requirements class going to be added in #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions

btw This constants already moved in #1987824: Convert system_php() to a new style controller

so here needs
- find a place for new constants (REQUIREMENT_INFO is not in SystemManager) still think better to use Requirements class from #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions
- properly deprecate them
- replace usage
- file change record
- not sure about how to add deprecated tests for constants

Just to make ball roll & check how much affected in tests

CR drafted https://www.drupal.org/node/3047376
Also rerolled the patch for 8.8.x
Should we also replace the usage of the deprecated constants as part of this issue or will it be handled in follow-up?

Deprecation should include replacement of usage.

Replaced the usage of deprecated constants.

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -898,7 +899,7 @@ function hook_updater_info_alter(&$updaters) {
- * If a requirement has a severity of REQUIREMENT_ERROR, install.php will abort
+ * If a requirement has a severity of Requirements::SEVERITY_ERROR, install.php will abort

@@ -912,7 +913,7 @@ function hook_updater_info_alter(&$updaters) {
- * Moreover, any requirement with a severity of REQUIREMENT_ERROR severity will
+ * Moreover, any requirement with a severity of Requirements::SEVERITY_ERROR severity will

This two lines reached 80 chars limit.

Adding some tags to hopefully get more eyes on this today in seattle.

Rerolled the patch & comments addressed in #11.

@yogeshmpawar
Thanks for rerolling. please provide the interdiff.

@naveenvalecha - your patch failed to apply on 8.8.x branch.

@naveenvalecha - Please find the attached interdiff file between #10 & #15.

+++ b/core/lib/Drupal/Core/Requirements.php
@@ -0,0 +1,30 @@
+  /**
+   * Requirement severity -- Informational message only.
+   */
+  const SEVERITY_INFO = -1;

sorry if I'm repeating a discussion that happened before.

I'm wondering if we still need the "Requirement severity --" prefix here. Seems pretty self-explaining, we are on the requirements class and the constant is prefixed with SEVERITY_..

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -898,8 +899,8 @@ function hook_updater_info_alter(&$updaters) {
  * As a consequence, install-time requirements must be checked without access
  * to the full Drupal API, because it is not available during install.php.
- * If a requirement has a severity of REQUIREMENT_ERROR, install.php will abort
- * or at least the module will not install.
+ * If a requirement has a severity of Requirements::SEVERITY_ERROR, install.php
+ * will abort or at least the module will not install.

@@ -932,10 +933,10 @@ function hook_updater_info_alter(&$updaters) {
  *   - severity: The requirement's result/severity level, one of:
- *     - REQUIREMENT_INFO: For info only.
- *     - REQUIREMENT_OK: The requirement is satisfied.
- *     - REQUIREMENT_WARNING: The requirement failed with a warning.
- *     - REQUIREMENT_ERROR: The requirement failed with an error.
+ *     - Requirements::SEVERITY_INFO: For info only.
+ *     - Requirements::SEVERITY_OK: The requirement is satisfied.
+ *     - Requirements::SEVERITY_WARNING: The requirement failed with a warning.
+ *     - Requirements::SEVERITY_ERROR: The requirement failed with an error.
  */

In hook documentation/examples, we usually use the full namespace for classes, same for docs.

At the same time, repeating the constants here seems not that useful, especially since we duplicate the documentation with less information than on the class. I'm wondering if it would be sufficient to just say "One of the severity constants of the \Drupal\Core\Requirements class".

@berdir asked me to comment on his review in #20:
- I agree that we probably don't need to have "Requirement severity --" in documentation of a constant called Requirements::SEVERITY_* in a Requirements class. But maybe we could make the messages clearer, something like "Indicates an informational message from a requirements check" and "Indicates the requirement is satisfied"?
- It is true that all class mentions in documentation should have namespaces.
- I agree with the suggestion of not duplicating documentation too -- it makes it hard to maintain. So, "One of the severity constants of the \Drupal\Core\Requirements class" for the hook docs sounds good.... Or maybe "One of the SEVERITY_* constants from the \Drupal\Core\Requirements class" would be even clearer?

I also took a quick look at the patch in #15, and have a few other comments:
a) Make sure to use the namespace in docs in other spots in core/lib/Drupal/Core/Extension/module.api.php too

b)

+++ b/core/lib/Drupal/Core/Requirements.php
@@ -0,0 +1,30 @@
+<?php
+
+namespace Drupal\Core;
+
+/**
+ * Defines requirements related constants for use with Drupal.
+ */

Should be "requirements-related" (hyphenated).

c)

+++ b/core/modules/system/src/SystemManager.php
@@ -7,6 +7,7 @@
 use Drupal\Core\Menu\MenuLinkInterface;
 use Drupal\Core\Menu\MenuTreeParameters;
 use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\Requirements;
 use Symfony\Component\HttpFoundation\RequestStack;
 
 /**
@@ -52,17 +53,17 @@ class SystemManager {
   /**
    * Requirement severity -- Requirement successfully met.
    */
-  const REQUIREMENT_OK = 0;
+  const REQUIREMENT_OK = Requirements::SEVERITY_OK;
 
   /**
    * Requirement severity -- Warning condition; proceed but flag warning.
    */
-  const REQUIREMENT_WARNING = 1;
+  const REQUIREMENT_WARNING = Requirements::SEVERITY_WARNING;
 
   /**
    * Requirement severity -- Error condition; abort installation.
    */
-  const REQUIREMENT_ERROR = 2;
+  const REQUIREMENT_ERROR = Requirements::SEVERITY_ERROR;
 

This seems like very weird code to me. Why is the Requirements class final, when it looks like this class really wants to extend it? Or should this class just use the constants from the other class and deprecate those class members? But maybe you discussed this already, I haven't read the other issue comments.

a) Make sure to use the namespace in docs in other spots in core/lib/Drupal/Core/Extension/module.api.php
Reviewed. I think it's ok.

b) Should be "requirements-related" (hyphenated).
Changed

c)Why is the Requirements class final
Any reason to extend this class?

Sorry for uncompatible IDEA patch file. Reroll with clean diff.

Fixed missing Requirements.php in patch file. Reroll.

All looks good so setting back to Needs Review! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).

+++ b/core/lib/Drupal/Core/Requirements.php
@@ -0,0 +1,30 @@
+  /**
+   * Requirement severity -- Informational message only.
+   */
...
+  /**
+   * Requirement severity -- Requirement successfully met.
+   */
...
+  /**
+   * Requirement severity -- Warning condition; proceed but flag warning.
+   */
...
+  /**
+   * Requirement severity -- Error condition; abort installation.
+   */

At least : or - would be better than --

Also, Requirement severity sounds weird. I think Severity level will be more clear there. Because code reads exactly like Requirements::SEVERITY_<level>

Addressed comment in #28 & added interdiff as well.

Typically, : used without blank space before the symbol.

Addressed comment in #31 & added interdiff as well.

All looks good so setting back to Needs Review! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).

Looks good to me. +1 for rtbc

In core/lib/Drupal/Core/Extension/module.api.php ...

Technically, if you refer to a class in documentation, you are supposed to give its full namespace. So instead of

+ * If a requirement has a severity of Requirements::SEVERITY_ERROR, install.php
+ * will abort or at least the module will not install.

it should say \Drupal\Core\Requirements::SEVERITY_ERROR.

Similarly elsewhere in the patch. See
https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
for where this documentation standard is written down.

you are supposed to give its full namespace

changed.

Reupload

Reupload

Thanks for the new patch!

A few notes:

a) When you make a new patch on an issue that already had a patch, please make an interdiff file. I made one this time (see attached). Here are instructions on how to make an interdiff:
https://www.drupal.org/documentation/git/interdiff

b) When you attach a patch, set the issue status to "Needs review". That will automatically launch a test (I assume you did one manually?) and alert reviewers that there is a patch to review.

c) Regarding the new patch, all documentation needs to wrap at under-80 character lines. Several of the lines you added are now quite a bit longer than 80 lines, so they need to be rewrapped. So, leaving the status at "Needs work".

As a note, when wrapping lines within a list, the wrapped section should be indented so that the text starts at the same place as the previous line. So it will look something like this (but with lines extending to as close to 80 characters as possible):

 * Some documentation that starts a list:
 * - List item 1. Assume this was long
 *   and needed to be wrapped. And started a sublist:
 *   - Sublist item 1. Assume this was long too
 *     and also needed to be wrapped.

Looks like the rest.install does not use requirements anymore. Removed it's changes from the patch. Fixed comments.

BTW, maybe we should make an interface instead of a final class? Sorry, if it's documented somewhere and I missed it.

Here's the same as above, but using an interface. Just in case.

+1 for the interface.
Still wondering... Should the SEVERITY be a part of the interface name? The Requirements name sounds generic and on the other hand RequirementsSeverity (or something similar) - more specific.

Oh, just ignore the #45 thoughts about naming. Anyway, this will be merged with other constants into a single Requirements class (regarding to #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions) And regarding interface, that requires to follow the CS of the interface naming with suffix "Interface". So I don't think that it would look great.

Tested the #44 patch. It does not apply anymore. Reroll.

Rerolled and removed some changes to content_moderation.install and workspaces.install as those requirements were removed! Also added 1 that was missing from media.install. No interdiff due to the considerable difference in the reroll.

I've checked phpcs and manually checked all documentation added to ensure no lines are longer than 80 characters so this should be good to RTBC now!

EDIT: CR was drafted back in #8 https://www.drupal.org/node/3047376

LGTM!

+1. Docs look good.

In #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions I've pushed back on adding \Drupal\Core\Requirements because it's a very generic namespace that risks making core/lib/Drupal/Core a dumping ground for things that we don't know where to put.

I have a couple of thoughts on this issue.

I feel this is OO-ification for OO-ification's sake. We're only moving the constants so they can be autoloaded. But a proper redesign of the requirements system might end up with Requirement value objects that makes these constants redundant - imagine $requirement->isError() etc...

If what we want to solve autoloadability of constants there's another way. We could add core/constants.php to our autoloader in the files section and repeating myself from the other issue the advantages are

• there's no deprecation
• the constants become available to autoloaded code
• as constants.php grows it becomes a place to learn about all the core provided constants

The one downside is this approach is only available to core and not core modules/themes, or contrib or custom because they don't using composer to set up their autoloading. But I'm not sure that outweighs the advantages listed.

I'm adding the Needs issue summary update tag so that the issue summary includes the different approaches suggested by @alexpott in comment #50.

The patch needs a reroll.

Re-roll patch created for 9.1.x

Ignore previous one, Please review this.

@vsujeetkumar You have some invalid PHP.

PHP Parse error:  syntax error, unexpected '}' in /var/www/html/core/modules/system/system.install on line 337
xargs: php: exited with status 255; aborting

For local development, you can check for valid PHP on any files that are have been modified from the 9.1.x branch using the following handy snippet:

git diff 9.1.x --name-only --diff-filter=AM | grep 'php' | xargs -n1 -P8 php -l

Fixed php syntax issue, Please review.

@vsujeetkumar it doesn't look like #59 is a reroll of #47?

+++ b/core/includes/install.inc
@@ -11,28 +11,52 @@
+ * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
+ *   Use \Drupal\Core\Requirements::SEVERITY_WARNING instead.

This message is now wrong. I believe it should be "deprecated in 9.1.0 and removed in 10.0.0"? I could be wrong though...

See https://www.drupal.org/core/deprecation#how-method

Also alexpott's comment in #50 has still not been addressed, so there is not really a lot of point in rerolling this patch over and over until we figure out what we're actually doing.

Repair status

Symfony has its own requirement checker https://github.com/symfony/requirements-checker

What about to implement requirement checks based on the Symfony package?

Re #70 this class looks outdated https://github.com/symfony/requirements-checker/blob/master/src/SymfonyR...

Moreover it will need to override more then it provides

Removed needs reroll tag.

Given comment #50, this is not a Novice task. Also updating issue summary with note from #50.

Nowadays idea of discovery Requirement objects could use attributes to define title, value and description.
Other similar kind of checks/tasks install system has, so approach from #50 sounds viable

+++ b/core/lib/Drupal/Core/Render/Element/StatusReport.php
@@ -35,13 +37,13 @@ public static function preRenderGroupRequirements($element) {
     $severities = static::getSeverities();
...
-      $severity = $severities[REQUIREMENT_INFO];
+      $severity = $severities[Requirements::SEVERITY_INFO];
...
-        $severity = $severities[REQUIREMENT_OK];
+        $severity = $severities[Requirements::SEVERITY_OK];
...
       $grouped_requirements[$severity['status']]['title'] = $severity['title'];

+++ b/core/lib/Drupal/Core/Requirements.php
@@ -0,0 +1,30 @@
+final class Requirements {

it could be enum but it actually used only to group requirements

Seems like we have an issue that already has some work done in that way #2909472: Add value objects to represent the return of hook_requirements

Looks yep!

I am going to work on this because we need these constants for hook_update_requirements and hook_runtime_requirements.

The one downside is this approach is only available to core and not core modules/themes, or contrib or custom because they don't using composer to set up their autoloading. But I'm not sure that outweighs the advantages listed.

Since these classes are used in core, contrib, and custom, I think this disqualifies the constants.php approach.

However, we don't actually have to deprecate these here, I think we can do that when we deprecate hook_requirements, for now we can just create the new classes and just use them as we convert hooks.

Deprecation suppose removal of the usage, 240LOC surely needs chunking

git grep REQUIREMENT_ |wc -l
241

Yes, I wouldn't create the deprecation here, just create the new landing place for constants and the new constants we need.

I wonder if there is a way to have both.

E,g. something like
/Drupal/Core/Constants
That is where the constants live, you can use it if you need it.
Maybe it makes sense to auto load it so they are available everywhere that uses autoloading.

I am not going to work on this at the moment, I think @berdir found a clean workaround, I think my comments about constants.php are still valid.

I keep adding @todo comments to core MRs with @see pointing here. We really need a better solution than manually including install.inc, or worse, using loadInclude($module, 'install') to get it to happen magically. 😂

I'm working on rebasing this. Now most of the hooks are converted to OOP we can see what is left over.

Oops commented on wrong issue. I'm actually working on rebasing #3410938: Create enums for RequirementSeverity and deprecate drupal_requirements_severity() constants

Should we close this issue in favour of #3410938: Create enums for RequirementSeverity and deprecate drupal_requirements_severity() constants ?

I think this can be closed as #3410938: Create enums for RequirementSeverity and deprecate drupal_requirements_severity() constants went in which deprecates the REQUIREMENT_* constants.

thank you!