Convert hook requirements that do not interact with install time

I'm working on this.

Migrate tests are failing due to update I think.

We need the constants!

Found a way forward from @berdir with modulehandler loadInclude.

I am getting navigation performance test failures in this, I'm fairly sure this is unrelated:

PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.10
Configuration: /var/www/html/phpunit.xml

F                                                                   1 / 1 (100%)

Time: 00:23.234, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest::testLogin
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     0 => 'SELECT "session" FROM "sessions" WHERE "sid" = "SESSION_ID" LIMIT 0, 1',
     1 => 'SELECT * FROM "users_field_data" "u" WHERE "u"."uid" = "2" AND "u"."default_langcode" = 1',
     2 => 'SELECT "roles_target_id" FROM "user__roles" WHERE "entity_id" = "2"',
-    3 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"',
+    3 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.maintenance_mode" ) AND "collection" = "state"',
+    4 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "routing.non_admin_routes" ) AND "collection" = "state"',
+    5 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"',
+    6 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "twig_extension_hash_prefix" ) AND "collection" = "state"',
+    7 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.private_key" ) AND "collection" = "state"',
+    8 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "asset.css_js_query_string" ) AND "collection" = "state"',
+    9 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "drupal.test_wait_terminate" ) AND "collection" = "state"',
+    10 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.cron_last" ) AND "collection" = "state"',
+    11 => 'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("state:Drupal\Core\Cache\CacheCollector", "LOCK_ID", "EXPIRE")',
+    12 => 'DELETE FROM "semaphore"  WHERE ("name" = "state:Drupal\Core\Cache\CacheCollector") AND ("value" = "LOCK_ID")',
 ]

/var/www/html/core/modules/navigation/tests/src/FunctionalJavascript/PerformanceTest.php:73

FAILURES!
Tests: 1, Assertions: 4, Failures: 1.

Performance test was fixed in 🐛 Performance tests need to run cron Active

Started reviewing by responding to (and resolving) the open threads. Haven't yet otherwise really reviewed the MR changes, although an initial glance looks good. Hope to review this in more depth on Tuesday.

So from what I can tell it's majority moving things around and adding dependency injection and string translation stuff (nice!).

I did have 2 questions on the MR that @dww addressed.

Don't see anything wrong here. +1 for RTBC but will refers to @dww review in #12

I agree that this looks good based on earlier reviews I did.

My only uncertainty is in which order we want to proceed. We already completely break BC on modules *calling* requirements hooks. This is somewhat rare but it happens, I of course have a module that does that ( https://www.drupal.org/project/monitoring → , see also https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22modul..., without moduleHandler it finds a lot more, but I assume that's all D7 code).

That's OK I think, this isn't meant to be an API, and I do plan to update monitoring.

But as I mentioned in 📌 Add value objects to represent the return of hook_requirements Needs work , the new hooks gives us a chance to considerably simplify the BC layer that the new value object for requirements definitions needs to support. we could say that the new hooks must return value objects. The it might make sense to do the other issue issue first. But that isn't close yet and there are more BC things to consider (for example usage of #type status_report, which is also used in a few projects, including again one of mine: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22%27%2...).

Long story short, it's probably OK to get this in now and first. Might want to think again about it before we do system module though.

Nearly RTBC. I just opened a ton of threads, the majority of which are pedantic nits, an out of scope question about a possible followup, and some suggestions that might not be in scope or viable. 😅 Only a couple of points of real substance, but all trivial / easy fixes.

Thanks!
-Derek

p.s. Initial saving of credits - so far everyone contributed meaningfully.

I applied everything and responded to the other questions.

I'll keep an eye on tests.

Don't love the loadInclude() hack, but as a transitional solution, I suppose it's fine if we improve the comments about it.

Almost there!

Thanks,
-Derek

Yeah it's a weird edge case, once the constant move is complete we'll have a better solution.

Thanks for fixing that up. I see nothing else to improve. Pipeline is green. Code is clean and good. Mostly just moving things around, with a few slight improvements (direct links to evergreen docs, better comments). Title and summary are clear. No need for a CR or release note. RTBC!

Oh yeah, tagging as a 11.2 priority.

@dww I agree with #19 having reviewed the code. But i have added one recommendation to the MR.

Opened the follow-up: 📌 Improve wording of Navigation requirements regarding Toolbar Active . @oily: Please continue the discussion there.

Thanks!
-Derek

Thank you @dww for doing the follow-up.

phpcs failed after a rebase.

PHPCS fixed

Rebase seems fine.

Conflicts with HEAD

Rebased, only conflict was with the helper function for update.install from this issue: 📌 Remove UI and routes for the ability to update modules and themes via update.module and authorize.php Active I copied the code for that function again to ensure the update was correct since this is a conversion issue.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

nicxvan → changed the visibility of the branch 3500632-convert-hook-requirements to hidden.

Rebased and pulled in node and update requirements changes.