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

CommentFileSizeAuthor
#4 3593792-nr-bot_te7_5xm8.txt867 bytesneeds-review-queue-bot

Issue fork drupal-3593792

Command icon 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:

Comments

longwave created an issue. See original summary.

longwave’s picture

Title: Convert test routes to attributes » Convert system_test routes to attributes
Component: phpunit » system.module
Status: Active » Needs review

Scoped this down to system_test module only, added some self-review comments.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new867 bytes

The 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.

longwave’s picture

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed the MR by comparing all of the former routing.yml definitions 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.

mstrelan’s picture

Status: Reviewed & tested by the community » Needs work

Let'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 like drupalGetRoute(). Shall we open a separate issue to discuss this?

longwave-bot made their first commit to this issue’s fork.

longwave’s picture

Status: Needs work » Needs review
mstrelan’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good and everything passes.

  • catch committed 1e052355 on main
    task: #3593792 Convert system_test routes to attributes
    
    By: longwave
    By...
catch’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/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.

longwave’s picture

Status: Patch (to be ported) » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

The 11.x changes are identical to those that went into main with two exceptions:

  • There's one route in main that doesn't exist in 11.x.
  • There's one route in 11.x that doesn't exist in main.

I validated the route conversion that's new in 11.x. It was correctly converted. This looks good to me.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed 8d4531c4 on 11.x
    task: #3593792 Convert system_test routes to attributes
    
    By: longwave
    By...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.