Problem/Motivation

Split from #3584823: Convert system module routes to use attributes

These were split because they are more complex and harder to test that the first batch.

  • TimeZoneController
  • BatchController
  • EntityAutocompleteController
  • CsrfTokenController

These were postponed on #3585072: Move controller classes to Controller namespaces but have been moved here:

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

Also wondering if now that we don't need a routing.yml file it is possible to move some of these out of system module. For example, EntityAutocompleteController could move to the Drupal\Core\Entity\Controller namespace.

Steps to reproduce

Proposed resolution

  • Move CronController and FileDownloadController to the Drupal\system\Controller namespace
  • Add stubs to deprecate these classes. This is preferred over moved_classes for easier contrib compatibility.
  • Convert routing yaml definitions to PHP attributes

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3585079

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’s picture

mstrelan commented
Title: [PP-2] Convert remaining system module routes to use attributes » [PP-1] Convert remaining system module routes to use attributes

#3584823: Convert system module routes to use attributes is in. We can wait for #3585072: Move controller classes to Controller namespaces or we can possibly move the two affected classes in this issue. Or we can split this again and work on the 4 remaining classes that are in the correct namespace.

mstrelan opened merge request !15660

mstrelan’s picture

mstrelan commented
Title: [PP-1] Convert remaining system module routes to use attributes » Convert remaining system module routes to use attributes
Status: Postponed » Needs review

Will need a CR for the moved classes, the rest can be reviewed.

mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

Unfortunately moved_classes doesn't seem to work as advertised.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Needs work » Needs review

Swapped to stubs instead of moved_classes, it's much easier for contrib to deal with and support multiple versions.

nicxvan’s picture

nicxvan commented

Unfortunately moved_classes doesn't seem to work as advertised.

What exactly doesn't work?

mstrelan’s picture

mstrelan commented

@nicxvan it still causes phpstan to fail. But now on reflection, that's by design. But handling multiple versions in contrib is still painful due to mismatched phpstan errors, so I don't think it's worth it..

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left small comments on the MR but looks good! Honestly @mstrelan if you make those small changes you're probably good to self RTBC.

mstrelan’s picture

mstrelan commented
Status: Needs work » Reviewed & tested by the community
godotislate’s picture

godotislate
he/him
commented

I didn't find an issue to convert the system module _form routes to attributes. so maybe they could be done here if there isn't one. OTOH there might be too many form entries in system.routing.yml and make the scope too big, so creating another issue is fine too.

Also wondering if now that we don't need a routing.yml file it is possible to move some of these out of system module. For example, EntityAutocompleteController could move to the Drupal\Core\Entity\Controller namespace.

This is a great idea. Discovery loops through the container namespaces, which include these in Drupal\Core:

   'Drupal\\Core\\Action' => 'core/lib/Drupal/Core/Action',
   'Drupal\\Core\\Block' => 'core/lib/Drupal/Core/Block',
   'Drupal\\Core\\Config' => 'core/lib/Drupal/Core/Config',
   'Drupal\\Core\\Datetime' => 'core/lib/Drupal/Core/Datetime',
   'Drupal\\Core\\DefaultContent' => 'core/lib/Drupal/Core/DefaultContent',
   'Drupal\\Core\\Entity' => 'core/lib/Drupal/Core/Entity',
   'Drupal\\Core\\Extension' => 'core/lib/Drupal/Core/Extension',
   'Drupal\\Core\\Field' => 'core/lib/Drupal/Core/Field',
   'Drupal\\Core\\Mail' => 'core/lib/Drupal/Core/Mail',
   'Drupal\\Core\\Menu' => 'core/lib/Drupal/Core/Menu',
   'Drupal\\Core\\Path' => 'core/lib/Drupal/Core/Path',
   'Drupal\\Core\\Plugin' => 'core/lib/Drupal/Core/Plugin',
   'Drupal\\Core\\Recipe' => 'core/lib/Drupal/Core/Recipe',
   'Drupal\\Core\\Render' => 'core/lib/Drupal/Core/Render',
   'Drupal\\Core\\TempStore' => 'core/lib/Drupal/Core/TempStore',
   'Drupal\\Core\\Theme' => 'core/lib/Drupal/Core/Theme',
   'Drupal\\Core\\TypedData' => 'core/lib/Drupal/Core/TypedData',
   'Drupal\\Core\\Validation' => 'core/lib/Drupal/Core/Validation',
   'Drupal\\Core\\Config\\Action' => 'core/lib/Drupal/Core/Config/Action',

I tested moving EntityAutocompleteController to Drupal\Core\Entity\Controller locally, and it seems to work correctly. I'm not sure if we should do that in this issue though, because maybe we'd want to move and update some corresponding tests as well. We can probably do a separate issue to evaluate what routes in system should be moved to Core and where.

One more thing is that we may want to wait for #3607968: Promote defaults._title to top level in route attributes so we use the Drupal Route attribute consistently.

mstrelan’s picture

mstrelan commented
Related issues: +#3608572: Convert system module form routes to use attributes

I didn't find an issue to convert the system module _form routes to attributes. so maybe they could be done here if there isn't one. OTOH there might be too many form entries in system.routing.yml and make the scope too big, so creating another issue is fine too.

#3608572: Convert system module form routes to use attributes

mstrelan’s picture

mstrelan commented

Opened #3615108: Move _title and _title_callback out of defaults section in existing route attributes and #3615109: Consider moving some routes and controllers out of system module so we don't need to hold this one up.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Will we move Cron and FileDownload into a core namespace in #3615109: Consider moving some routes and controllers out of system module - in which case should we have a further deprecation? Should we try to put things in their final place instead?

mstrelan’s picture

mstrelan commented

#14 that's a good point. I'm not sure how quickly #3615109: Consider moving some routes and controllers out of system module will move. Maybe we leave those two classes out of this issue?

godotislate’s picture

godotislate
he/him
commented

Agreed, let's skip Cron and FileDownload controllers here and address in #3615109: Consider moving some routes and controllers out of system module.

godotislate’s picture

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

NW for #14-16.