Problem/Motivation

Part of #3584794: Convert core routes to PHP attributes

Trying to see what is a good amount of scope to split this, and identify any patterns or problems with conversions.

Steps to reproduce

Proposed resolution

Convert the following:

  • Http4xxController
  • SystemController
  • PerformanceController
  • ThemeController
  • SystemInfoController
  • AdminController

Consider also converting the following, or splitting if scope is too large

  • TimeZoneController
  • BatchController
  • EntityAutocompleteController
  • CsrfTokenController

Split the following to another issue:

  • CronController - not in Controller namespace
  • FileDownloadController - not in Controller namespace

Leave form routes for after #3584793: Use PHP attributes for form route discovery

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3584823

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:

About issue forks

Comments

mstrelan created an issue. See original summary.

mstrelan opened merge request !15457

mstrelan’s picture

mstrelan commented
Status: Active » Needs review

Converted 19 routes in 2 classes. This feels like a good scope to get started, and I've posted a few comments and questions on the MR to highlight some of the quirks.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
mstrelan’s picture

mstrelan commented
Issue summary: View changes

Converted a few more

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I'm sorry I didn't finish the review last night before I went to bed.

My thoughts on reviewing this are:
These are really simple to review. They might make good Novice reviewer issues. Unless I've missed something important, then it's just a matter of comparing the new attribute to the old YAML route definition, then checking that it still works.

I'm comfortable with the current scope or continuing to add more. Personally, I would add in the other four classes noted as potentially being in-scope in the issue summary. Make it a nice, round 10 classes. I often tried to hit that mark when doing the mock object updates earlier in the year. @smustgrave might feel differently after all of those that he reviewed. But from my personal perspective this issue is way easier to understand than the mock object problems.

I'm going to set it to RTBC, but if you want to add more classes then I'll come back and check those too.

mstrelan’s picture

mstrelan commented

I think we should split off the remaining four classes as they seem harder to test and most have more requirements and options. They are not routes that you can simply hit in your browser, or access via one click, like those in ThemeController.

dcam’s picture

dcam commented

That sounds reasonable to me.

mstrelan’s picture

mstrelan commented

Opened #3585079: Convert remaining system module routes to use attributes for the split

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

One comment on the MR.

mstrelan’s picture

mstrelan commented

Fair comments, have adjusted and pipeline is green.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

lgtm

  • catch committed 548722a8 on main 
    task: #3584823 Convert system module routes to use attributes

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to main, thanks!

This needs a backport MR for 11.x

catch closed merge request !15457

mstrelan opened merge request !15606

mstrelan’s picture

mstrelan commented
Status: Patch (to be ported) » Needs review

Cherry-picked and resolved conflicts in SystemController

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Going off of https://git.drupalcode.org/project/drupal/-/commit/548722a899bad106ba420... looks like a good backport.

godotislate closed merge request !15606

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

There was a merge conflict in the 11.x MR with use statements in SystemController. Since it was just 1 line, I manually resolved.
Committed f2b26b6 and 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.

  • godotislate committed f2b26b61 on 11.x 
    task: #3584823 Convert system module routes to use attributes

By:...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

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.

Status: Fixed » Closed (fixed)

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