- Issue created by @nicxvan
- πΊπΈUnited States nicxvan
Migrate tests are failing due to update I think.
- πΊπΈUnited States nicxvan
Found a way forward from @berdir with modulehandler loadInclude.
- πΊπΈ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.
- πΊπΈ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.