Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Combines the following issues:
- #1987676: Convert database_test_db_query_temporary() to a new style controller
- #1987678: Convert database_test_even_pager_query() to a new style controller
- #1987680: Convert database_test_odd_pager_query() to a new style controller
- #1987682: Convert database_test_tablesort() to a new style controller
- #1987684: Convert database_test_tablesort_first() to a new style controller
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Beta phase evaluation
Issue category | Task because it's updating the code to the new API/code |
---|---|
Unfrozen changes | Unfrozen because it's a change in automated tests |
Disruption | Not disruptive because it only moves procedural code to class methods |
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff-21-25.txt | 1.99 KB | valthebald |
#25 | 2066557-25.patch | 9.63 KB | valthebald |
#22 | 2066557-21.patch | 9.48 KB | valthebald |
#15 | convert_system_module_s-2066557-15.patch | 13.42 KB | mparker17 |
Comments
Comment #1
mparker17Okay. After some adventures with fatal errors, I've created two patches.
Turns out the original test code made the assumption that it would only run inside it's test harness. Since my method for testing was "enable the hidden module with Drush and hit the menu paths", the result of
$query->execute()
would be null and the call to->fetchCol()
would subsequently fatal error.The
-1
patch is a straight conversion. The-1-a
improves the test so it doesn't fatal error if the query returns NULL. If the-1-a
patch passes all tests, that one should be preferred. If it fails, then just go with the -1.inter-interdiff.txt
shows the differences between two patches.Comment #2
mparker17Comment #3
mparker17Comment #5
mparker17Try these.
The
-5
versus-5-a
is the same as above.Since I made the same change to both
-5
and-5-a
, the interdiff between-1
versus-5
and-1-a
versus-5-a
is the same, I've included only one interdiff.Comment #6
dawehnerAll this db_select calls could be replaced directly by an injected db connection, so for example extending ControllerBase would just work.
Comment #7
dawehnerAll this db_select calls could be replaced directly by an injected db connection, so for example extending ControllerBase would just work.
Comment #8
sushylComment #9
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #10
sushylUnassigned, Couldn't spare enough time to look into it.
Comment #11
chakrapani CreditAttribution: chakrapani commentedThere has been some changes to the core and some of the changes from the earlier patches have been already committed(eg: removal of hook_menu).
Re-rolling the patch against latest head based on #5-a.patch.
comments from #6/#7 are not considered yet in the current patch.
The patch passed the tests when run locally.
Comment #12
xjm11: system-module.2066557-11-a.patch queued for re-testing.
Comment #14
xjmComment #15
mparker17Straight re-roll of #11.
I got a merge conflict...
... so I resolved it with
git rm core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php
Note the patch name has changed since #11 to whatever Dreditor recommended today.
Comment #16
mparker17Comment #18
BerdirThe reason it did conflict is that the other class moved to src/, and this has not yet been updated for that.
Comment #19
mparker17Comment #20
Mile23Comment #21
valthebaldComment #22
valthebaldComment #23
valthebaldLast patch just moves functions to Drupal\database_test\Controller\DatabaseTestController
Comment #24
Mile23Thanks, @valthebald!
Just some coding standards stuff:
Needs a blank line between
namespace
anduse
.All of these should be fully qualified like:
@return \Symfony\Component\HttpFoundation\JsonResponse
Comment #25
valthebaldHere we go
Comment #26
Mile23Super awesome. :-)
Comment #27
alexpottCommitted ed7f9d9 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Need to have a leading slash - fixed on commit.