Convert hook requirements that do not interact with install time

Created on 18 January 2025, 2 months ago

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

Convert

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

base system

Created by

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I'm working on this.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Merge request !10949Resolve #3500632 "Convert hook requirements" β†’ (Open) created by nicxvan
  • Pipeline finished with Failed
    2 months ago
    Total: 126s
    #399250
  • Pipeline finished with Failed
    2 months ago
    Total: 139s
    #399251
  • Pipeline finished with Failed
    2 months ago
    Total: 137s
    #399254
  • Pipeline finished with Failed
    2 months ago
    Total: 624s
    #399257
  • Pipeline finished with Failed
    2 months ago
    Total: 2004s
    #399266
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Migrate tests are failing due to update I think.

  • Pipeline finished with Failed
    2 months ago
    Total: 189s
    #399484
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Failed
    2 months ago
    Total: 3722s
    #399491
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    We need the constants!

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Found a way forward from @berdir with modulehandler loadInclude.

  • Pipeline finished with Failed
    2 months ago
    Total: 594s
    #399676
  • Pipeline finished with Running
    2 months ago
    #399688
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.
    
    
  • Pipeline finished with Success
    2 months ago
    Total: 579s
    #399805
  • Pipeline finished with Success
    2 months ago
    Total: 849s
    #400298
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Performance test was fixed in πŸ› Performance tests need to run cron Active

  • πŸ‡ΊπŸ‡ΈUnited States dww

    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.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    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.

  • πŸ‡ΊπŸ‡ΈUnited States dww

    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.

  • Pipeline finished with Canceled
    2 months ago
    Total: 87s
    #404196
  • Pipeline finished with Canceled
    2 months ago
    Total: 113s
    #404199
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I applied everything and responded to the other questions.

    I'll keep an eye on tests.

  • Pipeline finished with Canceled
    2 months ago
    Total: 135s
    #404202
  • Pipeline finished with Success
    2 months ago
    Total: 373s
    #404204
  • πŸ‡ΊπŸ‡ΈUnited States dww

    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

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

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

  • Pipeline finished with Failed
    2 months ago
    Total: 189s
    #404307
  • Pipeline finished with Success
    2 months ago
    Total: 2682s
    #404354
  • πŸ‡ΊπŸ‡ΈUnited States dww

    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!

  • πŸ‡ΊπŸ‡ΈUnited States dww

    Oh yeah, tagging as a 11.2 priority.

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

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

  • πŸ‡ΊπŸ‡ΈUnited States dww

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

    Thanks!
    -Derek

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Thank you @dww for doing the follow-up.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 188s
    #436333
  • First commit to issue fork.
  • πŸ‡¬πŸ‡§United Kingdom catch

    phpcs failed after a rebase.

  • Pipeline finished with Failed
    28 days ago
    Total: 86s
    #440905
  • Pipeline finished with Success
    28 days ago
    #440913
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    PHPCS fixed

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Rebase seems fine.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Conflicts with HEAD

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • Pipeline finished with Success
    20 days ago
    Total: 1142s
    #447990
  • Pipeline finished with Failed
    8 days ago
    Total: 514s
    #457190
Production build 0.71.5 2024