Problem/Motivation

Consider updating controllers with a single routable and consistently named method. Each controller would then represent a single route. This would helps prevent bloat in controllers, wherein dependencies and functionality are brought in but not utilized by all routes. It avoids the need to invent a new method name for each controller/route, with a common / standardized entry point. In the case where a route needs to be deprecated or moved, we can easily deprecate an entire class without worrying about side effects.

Part of the rationale for this is to make routing definitions look a little cleaner. Much like _form, which have represent something singular. Many instances of this however will be nullified by #3311365: Use PHP attributes for route discovery as they will no longer need a pointer to _controller. This will still be still useful for projects extending/reusing controllers, or creating routing entries by other means.

Proposed resolution

Update controllers to have a single routable method, via __invoke.
Deprecate existing methods.

Paired with #3311365: Use PHP attributes for route discovery, our controllers could only require this boilerplate:

#[Route('/test')]
final class TestController extends ControllerBase {
  public function __invoke(): Response|array {
    return ['#plain_text' => 'Hello world!'];
  }

Remaining tasks

Decide if we want to do this.

Many LOCs will be affected. Subsystem maintainer approval?

Depending on the size of task, it may make sense to break into subsystems.

Decide to do controllers with single methods AND controllers with multiple methods all in one go. Or, break into two: single first, then multiple. Perhaps dependent on quantity instances (TBD)

Most instances are for tests. Some of which have controllers with quite a lot of methods. Porting these will introduce a lot of extra files and boilerplate. Should we port these also?

In the case of controllers with multiple routeable methods, common functionality needs to be identified to be converted to traits, or other, on a case-by-case basis.

Implement.

User interface changes

Nil.

API changes

Many methods deprecated, immediately replaced with a consistently named method. Or moved to a different class entirly

Data model changes

Nil

Release notes snippet

TBD

Analysis

Analysis of controller/method, sorted by method count

Non test modules

Class Method Count Method Detail
\Drupal\big_pipe\Controller\BigPipeController:setNoJsCookie 1
\Drupal\block\Controller\BlockAddController 1 blockAddConfigureForm
\Drupal\block\Controller\BlockLibraryController 1 listBlocks
\Drupal\block\Controller\BlockListController 1 listing
\Drupal\block\Controller\CategoryAutocompleteController 1 autocomplete
\Drupal\ckeditor5\Controller\CKEditor5ImageController 1 upload
\Drupal\ckeditor5\Controller\CKEditor5MediaController 1 mediaEntityMetadata
\Drupal\config_translation\Controller\ConfigTranslationListController 1 listing
\Drupal\config_translation\Controller\ConfigTranslationMapperList 1 render
\Drupal\content_moderation\Controller\ModeratedContentController 1 nodeListing
\Drupal\contextual\ContextualController 1 render
\Drupal\Core\Entity\Controller\EntityRevisionViewController 1
\Drupal\editor\EditorController 1 filterXss
\Drupal\file\Controller\FileWidgetAjaxController 1 progress
\Drupal\filter\Controller\FilterController 1 filterTips
\Drupal\help_topics\Controller\HelpTopicPluginController 1 viewHelpTopic
\Drupal\image\Controller\ImageStyleDownloadController 1 deliver
\Drupal\layout_builder\Controller\AddSectionController 1 build
\Drupal\layout_builder\Controller\ChooseSectionController 1 build
\Drupal\layout_builder\Controller\MoveBlockController 1 build
\Drupal\media\Controller\MediaFilterController 1 preview
\Drupal\media\Controller\OEmbedIframeController 1 render
\Drupal\menu_link_content\Controller\MenuController 1 addLink
\Drupal\menu_ui\Controller\MenuController 1 getParentOptions
\Drupal\migrate_drupal_ui\Controller\MigrateController 1 showLog
\Drupal\node\Controller\NodePreviewController 1 view
\Drupal\shortcut\Controller\ShortcutSetController 1 addShortcutLinkInline
\Drupal\system\Controller\AdminController 1 index
\Drupal\system\Controller\BatchController 1 batchPage
\Drupal\system\Controller\CsrfTokenController 1 csrfToken
\Drupal\system\Controller\EntityAutocompleteController 1 handleAutocomplete
\Drupal\system\Controller\Http4xxController:on401 1
\Drupal\system\Controller\Http4xxController:on403 1
\Drupal\system\Controller\Http4xxController:on404 1
\Drupal\system\Controller\Http4xxController:on4xx 1
\Drupal\system\Controller\TimezoneController 1 getTimezone
\Drupal\system\FileDownloadController 1 download
\Drupal\system\MachineNameController 1 transliterate
\Drupal\taxonomy\Controller\TaxonomyController 1 addForm
\Drupal\toolbar\Controller\ToolbarController 1 subtreesAjax
\Drupal\tracker\Controller\TrackerController 1 buildContent
\Drupal\views\Controller\ViewAjaxController 1 ajaxView
\Drupal\views_ui\Form\Ajax\AddHandler 1 getForm
\Drupal\views_ui\Form\Ajax\Analyze 1 getForm
\Drupal\views_ui\Form\Ajax\ConfigHandler 1 getForm
\Drupal\views_ui\Form\Ajax\ConfigHandlerExtra 1 getForm
\Drupal\views_ui\Form\Ajax\ConfigHandlerGroup 1 getForm
\Drupal\views_ui\Form\Ajax\Display 1 getForm
\Drupal\views_ui\Form\Ajax\EditDetails 1 getForm
\Drupal\views_ui\Form\Ajax\Rearrange 1 getForm
\Drupal\views_ui\Form\Ajax\RearrangeFilter 1 getForm
\Drupal\views_ui\Form\Ajax\ReorderDisplays 1 getForm
media_library.ui_builder:buildUi 1
\Drupal\block\Controller\BlockController 2 demo, performOperation
\Drupal\block_content\Controller\BlockContentController 2 add, addForm
\Drupal\config\Controller\ConfigController 2 diff, downloadExport
\Drupal\contact\Controller\ContactController 2 contactSitePage, contactPersonalPage
\Drupal\field_ui\Controller\EntityDisplayModeController 2 viewModeTypeSelection, formModeTypeSelection
\Drupal\help\Controller\HelpController 2 helpMain, helpPage
\Drupal\history\Controller\HistoryController 2 getNodeReadTimestamps, readNode
\Drupal\layout_builder\Controller\ChooseBlockController 2 build, inlineBlockList
\Drupal\locale\Controller\LocaleController 2 checkTranslation, translatePage
\Drupal\search\Controller\SearchController 2 performOperation, setAsDefault
\Drupal\shortcut\Controller\ShortcutController 2 addForm, deleteShortcutLinkInline
\Drupal\system\Controller\SystemInfoController 2 status, php
\Drupal\system\CronController 2 run, runManually
\Drupal\update\Controller\UpdateController 2 updateStatus, updateStatusManually
\Drupal\book\Controller\BookController 3 bookRender, adminOverview, bookExport
\Drupal\dblog\Controller\DbLogController 3 overview, eventDetails, topLogMessages
\Drupal\node\Controller\NodeController 3 addPage, revisionOverview, revisionShow
\Drupal\system\Controller\ThemeController 3 uninstall, install, setDefaultTheme
\Drupal\forum\Controller\ForumController 4 forumIndex, forumPage, addContainer, addForum
\Drupal\system\Controller\SystemController 4 systemAdminMenuBlockPage, compactPage, themesPage, overview
\Drupal\user\Controller\UserAuthenticationController 4 resetPassword, login, loginStatus, logout
\Drupal\comment\Controller\CommentController 5 commentApprove, commentPermalink, getReplyForm, renderNewCommentsNodeLinks, redirectNode
\Drupal\views_ui\Controller\ViewsUIController 5 reportFields, reportPlugins, ajaxOperation, autocompleteTag, edit
\Drupal\user\Controller\UserController 7 logout, userPage, userEditPage, confirmCancel, resetPassLogin, resetPass, getResetPassForm

Test modules

Class Method Count Method Detail
\Drupal\advisory_feed_test\Controller\AdvisoryTestController 1 getPsaJson
\Drupal\block_test\Controller\TestMultipleFormController 1 testMultipleForms
\Drupal\ckeditor5_test\Controller\CKEditor5DialogTestController 1 testDialog
\Drupal\ckeditor5_test\Controller\CKEditor5OffCanvasTestController 1 testOffCanvas
\Drupal\comment_test\Controller\CommentTestController 1 commentReport
\Drupal\config_test\SchemaListenerController 1 test
\Drupal\container_rebuild_test\TestController 1 showModuleInfo
\Drupal\content_moderation_test_local_task\Controller\TestLocalTaskController 1 methodWithoutUpcastNode
\Drupal\contextual_test\Controller\TestController 1 render
\Drupal\csrf_test\Controller\TestController 1 testMethod
\Drupal\default_format_test\DefaultFormatTestController 1 content
\Drupal\deprecation_test\DeprecatedController 1 deprecatedMethod
\Drupal\error_service_test\Controller\LonelyMonkeyController 1 testBrokenClass
\Drupal\help_topics_test\Controller\HelpTopicsTestController 1 testPage
\Drupal\httpkernel_test\Controller\TestController 1 get
\Drupal\jqueryui_library_assets_test\Controller\JqueryUiTestAssetsController 1 build
\Drupal\js_deprecation_test\Controller\JsDeprecationTestController 1 jsDeprecationTest
\Drupal\js_errors_test\Controller\JsErrorsTestController 1 jsErrorsTest
\Drupal\js_message_test\Controller\JSMessageTestController 1 messageLinks
\Drupal\js_once_test\Controller\JsOnceTestController 1 onceTest
\Drupal\js_webassert_test\Controller\TestController 1 page
\Drupal\migrate_no_migrate_drupal_test\Controller\ExecuteMigration 1 execute
\Drupal\node_access_test_auto_bubbling\Controller\NodeAccessTestAutoBubblingController 1 latest
\Drupal\path_encoded_test\Controller\PathEncodedTestController 1 simple
\Drupal\position_shim_test\Controller\PositionShimTestController 1 build
\Drupal\position_shim_test\Controller\PositionShimTestPortedJqueryTestsController 1 build
\Drupal\system_test\Controller\PageCacheAcceptHeaderController 1 content
\Drupal\tabbable_shim_test\Controller\TabbableShimDialogIntegrationTestController 1 build
\Drupal\tabbable_shim_test\Controller\TabbableShimTestController 1 build
\Drupal\tabbingmanager_test\Controller\TabbingManagerTestController 1 build
\Drupal\update_script_test\Controller\UpdateScriptTestController 1 databaseUpdatesMenuItem
\Drupal\views_test_data\Controller\ViewsTestDataController 1 errorFormPage
\Drupal\views_test_data\Controller\ViewsTestFormMultipleController 1 testPage
\Drupal\views_test_modal\Controller\TestController 1 modal
\Drupal\views_test_render_cache\Controller\ViewsTestRenderCacheController 1 double
\Drupal\vocabulary_serialization_test\VocabularySerializationTestController 1 vocabularyResponse
chop 1
Drupal\image_lazy_load_test\Controller\ImageLazyLoadController 1 renderImage
\Drupal\basic_auth_test\BasicAuthTestController 2 modifyState, readState
\Drupal\big_pipe_regression_test\BigPipeRegressionTestController 2 regression2678662, regression2802923
\Drupal\cache_test\Controller\CacheTestController 2 urlBubbling, bundleTags
\Drupal\config_test\ConfigTestController 2 enable, disable
\Drupal\csrf_race_test\Controller\TestController 2 getCsrfToken, testMethod
\Drupal\form_test\Controller\FormTestController 2 twoFormInstances, storageLegacyHandler
\Drupal\media_test_oembed\Controller\ResourceController 2 get, getThumbnailWithNoExtension
\Drupal\menu_test\Controller\MenuTestController 2 menuTestCallback, themePage
\Drupal\pager_test\Controller\PagerTestController 2 multiplePagers, queryParameters
\Drupal\render_array_non_html_subscriber_test\RenderArrayNonHtmlSubscriberTestController 2 rawString, renderArray
\Drupal\test_page_test\Controller\TestPageTestController 2 testPage, testPageVarDump
\Drupal\tour_test\Controller\TourTestController 2 tourTest1, tourTest2
\Drupal\trusted_hosts_test\Controller\TrustedHostsTestController 2 fakeRequestHost, bagType
\Drupal\twig_extension_test\TwigExtensionTestController 2 testFilterRender, testFunctionRender
\Drupal\update_test\Controller\UpdateTestController 2 updateError, updateTest
Drupal\token_test\Controller\TestController 2 tokenReplace, tokenReplaceWithoutPassedBubbleableMetadata
\Drupal\big_pipe_test\BigPipeTestController 3 test, nope, multiOccurrence
\Drupal\common_test\Controller\CommonTestController 3 typeLinkActiveClass, destination, jsAndCssQuerystring
\Drupal\entity_test\Controller\EntityTestController 3 listReferencingEntities, listEntitiesAlphabetically, listEntitiesEmpty
\Drupal\help_page_test\HelpPageTestController 3 hasHelp, noHelp, testArray
\Drupal\language_test\Controller\LanguageTestController 3 typeLinkActiveClass, testSubRequest, testEntity
\Drupal\module_test\Controller\ModuleTestController 3 hookDynamicLoadingInvoke, hookDynamicLoadingInvokeAll, testClassLoading
\Drupal\paramconverter_test\TestControllers 3 testUserNodeFoo, testNodeSetParent, testEntityLanguage
\Drupal\conneg_test\Controller\TestController 4 simple, html, format, variable
\Drupal\database_test\Controller\DatabaseTestController 4 pagerQueryEven, pagerQueryOdd, testTablesort, testTablesortFirst
\Drupal\dialog_renderer_test\Controller\TestController 4 linksDisplay, modalContent, modalContentLink, modalContentInput
\Drupal\off_canvas_test\Controller\TestController 4 linksDisplay, thing1, thing2, otherDialogLinks
\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController 4 messagesPlaceholderFirst, messagesPlaceholderMiddle, messagesPlaceholderLast, queuedMessages
\Drupal\router_test\TestContent 4 test1, test11, subrequestTest, testAccount
\Drupal\error_test\Controller\ErrorTestController 5 generateWarnings, generateFatals, triggerException, triggerPDOException, triggerRendererException
\Drupal\render_attached_test\Controller\RenderAttachedTestController 5 teapotHeaderStatus, header, head, htmlHeaderLink, feed
\Drupal\dynamic_page_cache_test\DynamicPageCacheTestController 7 response, cacheableResponse, html, htmlWithCacheContexts, htmlUncacheableMaxAge, htmlUncacheableContexts, htmlUncacheableTags
\Drupal\menu_test\TestControllers 7 testLogin, test1, test2, testSession, testContextual, testDerived, testDefaults
\Drupal\batch_test\Controller\BatchTestController 8 testRedirect, testLargePercentage, testNestedDrupalFormSubmit, testNoForm, testFinishRedirect, testProgrammatic, testThemeBatch, testTitleBatch
\Drupal\ajax_test\Controller\AjaxTestController 10 dialogContents, renderTypes, dialog, insertLinksBlockWrapper, insertLinksInlineWrapper, dialogClose, render, theme, order, renderError
\Drupal\theme_test\ThemeTestController 12 testTemplate, testInlineTemplate, testSuggestion, testRequestListener, suggestionAlter, generalSuggestionAlter, suggestionProvided, specificSuggestionAlter, nonHtml, preprocessSuggestions, preprocessCallback, testThemeClass
\Drupal\twig_theme_test\TwigThemeTestController 12 phpVariablesRender, transBlockRender, placeholderOutsideTransRender, urlGeneratorRender, linkGeneratorRender, urlToStringRender, fileUrlRender, attachLibraryRender, registryLoaderRender, renderable, embedTagRender, dump
\Drupal\router_test\TestControllers 15 test1, test2, test3, test4, test7, test8, test9, test10, test18, test21, test23, test24, test25, test, testRouteName
\Drupal\session_test\Controller\SessionTestController 15 get, getFromSessionObject, getId, getIdFromCookie, set, noSet, setMessage, setMessageButDoNotSave, isLoggedIn, traceHandler, getSession, setSession, setSessionBagFlag, clearSessionBagFlag, hasSessionBagFlag
\Drupal\test_page_test\Controller\Test 15 renderTitle, staticTitle, controllerWithCache, httpResponseException, error, setHeader, renderEncodedMarkup, renderPipeInLink, escapedCharacters, escapedScript, unescapedScript, metaRefresh, renderPageWithDuplicateIds, renderPageWithoutDuplicateIds, deprecations
\Drupal\early_rendering_controller_test\EarlyRenderingTestController 16 renderArray, renderArrayEarly, ajaxResponse, ajaxResponseEarly, response, responseEarly, responseWithAttachments, responseWithAttachmentsEarly, cacheableResponse, cacheableResponseEarly, domainObject, domainObjectEarly, domainObjectWithAttachments, domainObjectWithAttachmentsEarly, cacheableDomainObject, cacheableDomainObjectEarly
\Drupal\system_test\Controller\SystemTestController 22 mainContentFallback, messengerServiceTest, statusMessagesForAssertions, lockAcquire, lockExit, lockPersist, system_test_cache_tags_page, system_test_cache_maxage_page, authorizeInit, setHeader, shutdownFunctions, requestDestination, getDestination, permissionDependentContent, respondWithResponse, respondWithPublicResponse, respondWithCacheableResponse, getCurrentDate, getTestHeader, simpleEcho, getCacheableResponseWithCustomCacheControl, getInstallProfile

Issue fork drupal-3317792

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

dpi created an issue. See original summary.

dpi’s picture

Title: Make controllers invokeable for DX and maintainability » 3317792-make-controllers-invokeable

Attached an example for Media.

dpi’s picture

Title: 3317792-make-controllers-invokeable » Make controllers invokeable for DX and maintainability
cilefen’s picture

I mostly code in other frameworks and I've often wondered what is the ideal number of route handlers per controller class. It could be "one".

dpi’s picture

As @Tim Plunkett mentioned in Slack, this is about setting and adapting to a policy, rather than adding a *new feature*.

catch’s picture

Having only one title and method on controllers seems sensible - I'm assuming we'd adopt this as core policy/coding standards but not actually enforce it on contrib/custom code?

For splitting it up, doing all single method controllers, then separately doing all multiple method controllers seems like a good split. If there's far too many of the latter we might need split again somehow.

dpi’s picture

Issue summary: View changes

I'm assuming we'd adopt this as core policy/coding standards but not actually enforce it on contrib/custom code?

Core-only policy sounds fine

For splitting it up, doing all single method controllers, then separately doing all multiple method controllers seems like a good split. If there's far too many of the latter we might need split again somehow.

Posted analysis to issue summary. Simply checked routing.yml files only, sorted by method count, and broken up between test and non test modules. I'm sure theres a few others not in routing.yml, like \Drupal\Core\Entity\Controller\EntityController, only referenced by route providers. In any case I think it informs the split here. Posted code used to build at https://github.com/dpi/core-controller-analysis

Seems like single-first is a good balance, even with test only modules. I think we can ignore multi-controller test modules as its going to be particularly noisy.

dpi’s picture

Tagging as suggested.

My opinion on coding standards for this is: its too obnoxious to apply to contrib.

andypost’s picture

Looks a nice idea! Although I more in favour of attributes to get rid of yaml for the most of such cases, I think we can just add invoke() api method for DX (no need to think on method name as controller has only one), that cases will be easier to catch woth sniffers

dpi’s picture

@andypost, route attribute can be tacked against a class, rather than a specific method, which I'd prefer for discoverability.

Edit: mentioned this to #3311365-6: Use PHP attributes for route discovery

dpi’s picture

Title: Make controllers invokeable for DX and maintainability » Make controllers invocable for DX and maintainability
Issue summary: View changes
andypost’s picture

btw Used to check is that approach "function class" is used and found invokable interface rfc and there was not a lot of dicussions

Discovery of attributes has the same cost for classes and its methods and it's interesting to compare with discovery by interface or even by base class kinda ControllerInvokableBase

PS ivocable vs invokable is DX--

dpi’s picture

Title: Make controllers invocable for DX and maintainability » Make controllers invokable for DX and maintainability

Ah, it was the “e” that made it look odd.

catch’s picture

Discovery of attributes has the same cost for classes and its methods and it's interesting to compare with discovery by interface or even by base class kinda ControllerInvokableBase

Yes we have plenty of controllers so this would be interesting to compare one vs the other.

longwave’s picture

Do we really want to enforce one class per route in core? There are examples in that list like EntityDisplayModeController and HelpController where on first glance it seems to make more sense to keep them together. If we allow both class and method level #[Route] attributes, it is then up to each implementation whether they have one single routed method or whether they have multiple routes per class.

dpi’s picture

HelpController injects three services. The services aren't used by both route methods, only one. There's no other sharing of state or code between them. It could be broken up easily.

EntityDisplayModeController has so much commonality it could be turned abstract, or optimised in other ways.

andypost’s picture

No reason to enforce as this only makes sense for "only-one-method-controllers"

helpController surely needs clean-up but for it there's #3037230: Finalize the merge of Help Topics into Help

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

wim leers’s picture

FYI: this is what we're doing in Experience Builder already (thanks to the CR at https://www.drupal.org/node/2997122). This is being formally documented in #3490069: [upstream] [PP-1] Auto-register XB's controllers' invokable services.

More importantly, on the core side of things, I suspect that #3311365: Use PHP attributes for route discovery is a soft blocker for this, because that will make the DX for this way better!

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.