Problem/Motivation

#3584793: Use PHP attributes for form route discovery allows us to use attributes for form route discovery. This issue is to convert routes in system.module

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3608572-nr-bot_xarr67tg.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3608572

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 !16222

mstrelan’s picture

mstrelan commented
Status: Active » Needs review

Used an LLM to help here. Have reviewed and take responsibility for the changes.

It's very straightforward, only two things to call out:

  1. system.modules_list is the only one with _title_context
  2. system.site_maintenance_mode has a compound permission (administer site configuration+administer software updates)
mstrelan’s picture

mstrelan commented

Added two more:

  1. system.theme_settings and system.theme_settings_theme share the same class, with service notation for the title callback
  2. system.prepare_modules_entity_uninstall is the most complex but shouldn't be an issue
akshay kashyap’s picture

akshay kashyap commented

I tested this MR locally on a Drupal 11.3.x installation.

After rebuilding caches, I verified that all of the converted form routes continue to be discovered correctly through PHP #[Route] attributes.

I tested the standard configuration forms, including Cron, Logging, Development settings, File system, Image toolkit, Regional settings and Maintenance mode. I also verified the Extend page, Theme settings, theme-specific settings, Module Uninstall and the entity uninstall route.

I specifically checked the edge cases mentioned in the latest updates:

  • system.modules_list still works correctly with _title_context.
  • system.site_maintenance_mode continues to respect the compound permission requirements.
  • system.theme_settings and system.theme_settings_theme load correctly, including the service-based title callback.
  • system.prepare_modules_entity_uninstall still honours the custom access check and dynamic title callback.

I also confirmed that the corresponding route definitions have been removed from system.routing.yml and are now discovered through PHP attributes without changing the existing behaviour.

The only review comment I noticed is the suggestion to use self::class instead of the fully qualified class name for the callback references. That looks like a reasonable cleanup, but I don't see it affecting the functionality of the MR.

Other than that, I didn't encounter any regressions during testing.

RTBC from my side.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Added a question about the title_context one. Perhaps also we should detect this in TitleResolver with an assertion.

@mstrelan also interested if you have any input into #3607968: Promote defaults._title to top level in route attributes

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Addressed feedback in #6, and converted callbacks in PrepareModulesEntityUninstallForm to use self::class

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3608572-nr-bot_xarr67tg.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.