- 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.
- πΊπΈ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!
-Derekp.s. Initial saving of credits - so far everyone contributed meaningfully.
- πΊπΈUnited States nicxvan
I applied everything and responded to the other questions.
I'll keep an eye on tests.
- πΊπΈ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.
- πΊπΈ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 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.
- First commit to issue fork.
- πΊπΈ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.