Problem/Motivation

hook_update_requirements() and hook_runtime_requirements() are now available.
Let's convert the core ones that only interact with update or runtime.

Runtime phase only

  • image
  • jsonapi
  • locale
  • mysql
  • navigation
  • node
  • search
  • update_test_schema
  • update
  • user
  • demo_umami

Update phase only

  • update_script_test

Steps to reproduce

N/A

Proposed resolution

Convert them to OOP equivalents

Remaining tasks

Fix migrate failures

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#34 3500632-nr-bot.txt91 bytesneeds-review-queue-bot
#30 3500632-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3500632

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

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented

I'm working on this.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !10949

nicxvan’s picture

nicxvan commented

Migrate tests are failing due to update I think.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Title: Convert hook requirements that do not interact with install time » [pp-1] Convert hook requirements that do not interact with install time
Related issues: +#2909480: Move REQUIREMENT_* constants out of install.inc file

We need the constants!

nicxvan’s picture

nicxvan commented
Title: [pp-1] Convert hook requirements that do not interact with install time » Convert hook requirements that do not interact with install time

Found a way forward from @berdir with modulehandler loadInclude.

nicxvan’s picture

nicxvan commented

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.
nicxvan’s picture

nicxvan commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented

Performance test was fixed in #3500489: Performance tests should uninstall automated_cron

dww’s picture

dww
we/he/they
commented
Related issues: +#2909472: Add value objects to represent the return of hook_requirements

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.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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 #2909472: Add value objects to represent the return of hook_requirements, 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.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

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.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I applied everything and responded to the other questions.

I'll keep an eye on tests.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

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

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

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

dww’s picture

dww
we/he/they
commented
Title: Convert hook requirements that do not interact with install time » Convert hook_requirements() that do not interact with install time
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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!

dww’s picture

dww
we/he/they
commented
Issue tags: +11.2.0 release priority

Oh yeah, tagging as a 11.2 priority.

oily’s picture

oily commented

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

dww’s picture

dww
we/he/they
commented

Opened the follow-up: #3502070: [PP-1] Improve wording of Navigation requirements regarding Toolbar. @oily: Please continue the discussion there.

Thanks!
-Derek

oily’s picture

oily commented

Thank you @dww for doing the follow-up.

catch made their first commit to this issue’s fork.

catch’s picture

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

phpcs failed after a rebase.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

PHPCS fixed

smustgrave’s picture

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

Rebase seems fine.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Conflicts with HEAD

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3500632-nr-bot.txt91 bytes

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 opened merge request !11896

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

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Rebased and pulled in node and update requirements changes.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3500632-nr-bot.txt91 bytes

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’s picture

nicxvan commented
Status: Needs work » Needs review
dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

I have not completely re-reviewed the entire MR changes tab. But I reviewed the individual commits since the last time I did so and everything seems in order. The pipeline is green again. Back to RTBC.

p.s. Noticed while re-reviewing, but we really need to fix #3122231: Singular variant for plural string must contain @count and formalize that. core/modules/node/src/Hook/NodeRequirements.php gets this wrong, but so does the original code in node.install.

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Needs review

Sorry to do this, but back to NR based on a new MR thread. I'm probably just confused, but I don't understand some stuff, so I want to retract my RTBC until I'm clear.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

I replied, setting back to RTBC after answering that question.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Left one suggestion on the MR
Reviewed each new hook class against the .install file in HEAD and it all looks good to me.

Leaving it at RTBC, please let me know if you're happy with the suggestion on the MR.

nicxvan’s picture

nicxvan commented

Applied! Thanks

  • larowlan committed f29806ce on 11.x 
    Issue #3500632 by nicxvan, dww, smustgrave, oily, berdir: Convert...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x - thanks!

larowlan closed merge request !11896

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

nicxvan closed merge request !10949