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
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
Comment #3
dpiAttached an example for Media.
Comment #4
dpiComment #5
cilefen commentedI 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".
Comment #6
dpiAs @Tim Plunkett mentioned in Slack, this is about setting and adapting to a policy, rather than adding a *new feature*.
Comment #7
catchHaving 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.
Comment #8
dpiCore-only policy sounds fine
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-analysisSeems 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.
Comment #9
dpiTagging as suggested.
My opinion on coding standards for this is: its too obnoxious to apply to contrib.
Comment #10
andypostLooks 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 sniffersComment #11
dpi@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
Comment #12
dpiComment #13
andypostbtw 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
ControllerInvokableBasePS ivocable vs invokable is DX--
Comment #14
dpiAh, it was the “e” that made it look odd.
Comment #15
catchYes we have plenty of controllers so this would be interesting to compare one vs the other.
Comment #16
longwaveDo 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.Comment #17
dpiHelpControllerinjects 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.EntityDisplayModeControllerhas so much commonality it could be turned abstract, or optimised in other ways.Comment #18
andypostNo reason to enforce as this only makes sense for "only-one-method-controllers"
helpControllersurely needs clean-up but for it there's #3037230: Finalize the merge of Help Topics into HelpComment #20
wim leersFYI: 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!