Problem/Motivation
Child of #3584794: [meta] Convert core routes to PHP attributes
Steps to reproduce
Proposed resolution
Routing definitions for test controllers will be moved to attributes on the controller where possible.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3593792
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:
- 3593792-11.x
changes, plain diff MR !16840
- 3593792-convert-test-routes
changes, plain diff MR !16201
Comments
Comment #3
longwaveScoped this down to system_test module only, added some self-review comments.
Comment #4
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.
Comment #5
longwaveComment #6
dcam commentedI reviewed the MR by comparing all of the former
routing.ymldefinitions to the migrated attributes one-by-one. I didn't find any issues. They all looked good to me. I didn't try to pull them up in the browser. I figured it was OK to assume that tests would fail if they didn't work correctly.I'm going to set the status to RTBC to indicate that this has been reviewed, but it may be prudent to wait to commit this pending the resolution of #3607968: Promote defaults._title to top level in route attributes.
Comment #7
mstrelan commentedLet's update this to use #3607968: Promote defaults._title to top level in route attributes. That is committed to main but needs an 11.x backport. I think we can just commit this to main, and only backport if the other issue is in.
I'm in two minds about @longwave's suggestion in the MR to omit the route name. Since these are only in a test module, removing these shouldn't be disruptive. I wouldn't recommend it for non-test routes, as it is useful for links, etc. The other side of this is that we could update tests that use
$this->drupalGet('system-test/main-content-fallback')to use something like$this->drupalGet(Url::fromRoute('system_test.main_content_fallback'))), or even have a helper likedrupalGetRoute(). Shall we open a separate issue to discuss this?Comment #9
longwaveComment #10
mstrelan commentedChanges look good and everything passes.
Comment #12
catchCommitted/pushed to main, thanks!
Will need an 11.x backport MR - not sure we really need to backport it to 11.x so we could also just mark it fixed against main, but moving for backport for now.
Comment #14
longwaveComment #15
dcam commentedThe 11.x changes are identical to those that went into main with two exceptions:
I validated the route conversion that's new in 11.x. It was correctly converted. This looks good to me.
Comment #16
catchCommitted/pushed to 11.x, thanks!