Issue summary last updated at comment 48 (2014-07-01)
Problem/Motivation
In the @EntityType annotation and it's descendants (ContentEntityType and ConfigEntityType), we group a bunch of things in a controllers
section. For example, in the Block entity class:
/**
* @EntityType(
* ...
* controllers = {
* "storage" = "Drupal\Core\Config\Entity\ConfigStorageController",
* "access" = "Drupal\block\BlockAccessController",
* "render" = "Drupal\block\BlockRenderController",
* "list" = "Drupal\block\BlockListController",
* "form" = {
* "default" = "Drupal\block\BlockFormController",
* "delete" = "Drupal\block\Form\BlockDeleteForm"
* }
* },
* ...
* )
*/
This grouping doesn't really make sense, because most of these objects aren't really Controllers in the MVC sense.
We really need a name to represent the concept of "thing that does X".
Note that we decided on calling some of these objects "handlers" in #1807042: Reorganizie entity storage/list/form/render controller annotation keys. However, the word handler doesn't help people understand anything about what the classes are doing. It obscures the fact that the responsibilities of each of these classes is actually pretty different in relation to the entity.
Proposed resolution
Keep the "storage" key but remove "Controller" from the names of the classes it references (#13, #22, #39): #2188613: Rename EntityStorageController to EntityStorageKeep the "form" key but remove "Controller" from the names of the classes it references (#13 and #22): #2238077: Rename EntityFormController to EntityFormRename the "list" key to "list_builder" and remove "Controller" from the names of the classes it references (#13 and #22, #38): #2120871-45: Rename EntityListController to EntityListBuilderRename the "render" key to "view_builder"and remove "Controller" from the names of the classes it references (#22, #23): #2097903: Rename EntityRenderController to EntityViewBuilder- Rename the "access" key to either "access_control" or "access_control_handler" (#13, #22, #29): #2154435: Rename EntityAccessController to EntityAccessControlHandler
- Move some of these keys to the top level of the
@*EntityType
annotation (i.e.: move them out of thecontrollers
grouping). - Decide whether to either:
Remaining tasks
- Decide what to do with the
controllers
grouping. - #2154435: Rename EntityAccessController to EntityAccessControlHandler
User interface changes
None.
API changes
/**
* @EntityType(
* ...
* controllers = {
* "storage" = "Drupal\Core\Config\Entity\ConfigStorageController",
* "access" = "Drupal\block\BlockAccessController",
* "render" = "Drupal\block\BlockRenderController",
* "list" = "Drupal\block\BlockListController",
* "form" = {
* "default" = "Drupal\block\BlockFormController",
* "delete" = "Drupal\block\Form\BlockDeleteForm"
* }
* },
* ...
* )
*/
... becomes something like...
/**
* @ConfigEntityType(
* ...
* "access_control_handler" = "Drupal\block\BlockAccessHandler",
* "view_builder" = "Drupal\block\BlockViewBuilder",
* "list_builder" = "Drupal\block\BlockListBuilder",
* "form" = {
* "default" = "Drupal\block\BlockForm",
* "delete" = "Drupal\block\Form\BlockDeleteForm"
* },
...
* )
*/
Original report by @msonnabaum
Splitting off from #1807042: Reorganizie entity storage/list/form/render controller annotation keys since that's now focussed on the annotation keys.
While I disagree that we need to change the name due to a conflict with Route Controllers (there are a few different, legitimate types of controllers), I do believe our naming here doesn't make much sense.
We are going back and forth on which bad name to choose, because none of them provide any more context to the reader about what these objects actually do. If anything, it just muddles their intention more.
Instead, how about we just dont use them?
/**
* Defines a Block configuration entity class.
*
* @EntityType(
* storage = "Drupal\block\BlockStorage",
* access = "Drupal\block\BlockAccessController",
* render = "Drupal\block\BlockRenderable",
* list = "Drupal\block\BlockList",
* form = {
* "default" = "Drupal\block\BlockForm"
* },
* )
*/
class Block extends ConfigEntityBase {
Compared to what we have now, I think these class names convey a lot more information about what they actually are (I only kept Controller on AccessController because "access control" is a for-real thing).
Comment | File | Size | Author |
---|---|---|---|
#76 | controllers-to-handlers-1976158-76.patch | 109.45 KB | Berdir |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedI think it helps for there to be some common name grouping these things. Otherwise, "access" and "label" are keys on the same level, but the former refers to a class/service while the latter refers to a string literal.
I thought consensus was fairly solid in #1807042: Reorganizie entity storage/list/form/render controller annotation keys on "handler". From a purely English standpoint (e.g., "a baggage handler"), I think that's a reasonable name. However, I'm not aware of it being a commonly adopted name in the software field. On Wikipedia, the pattern is named Servant.
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedI dont think we can say that there's a singular pattern here with both form and storage in there. Even if there was, naming classes after patterns is not ideal.
My main point is, the word handler here doesn't help you understand anything about what they are doing. It obscures the fact that the responsibilities of each of these classes is actually pretty different in relation to the entity. Renaming them handlers is a lateral move, it's not an improvement.
The list controller isn't a controller, it's a list. It is a thing itself that you render, that has a dependency on the entity it's listing. Form is similar.
Comment #3
yched CreditAttribution: yched commentedThe list controller isn't a list, it's a "thing that builds a list than you can render" ?
Similarly, the render controller isn't a renderable, it's a "thing that builds a renderable array" ?
Controller, handler - the important bit IMO is to have a word that represents that "thing that does X" indirection.
With a personal preference on "handler", precisely because it seems more agnostic than "controller".
"Handles", as in "takes care of", looks just fine to me.
Comment #4
msonnabaum CreditAttribution: msonnabaum commentedI'm not sure if returning a render array takes away from the object's identity, that's just a Drupalism that we have currently. I may be thinking forward a bit to what this would look like without render arrays.
I dont know that I'm up for a big argument about this, just thought I'd throw it out there. If no one likes it, feel free to close this or replace the summary with the previous proposal.
Comment #5
yched CreditAttribution: yched commentedSure, "render arrays" are a drupalism, but an object returning something that's "renderable" does qualify as being a "render handler", independently of the implementation details ?
Comment #6
msonnabaum CreditAttribution: msonnabaum commentedI'm just saying that the concept of a "render handler" is a difficult thing to reason about in a domain model. Really, it's a symptom of an anemic domain model.
Comment #7
yched CreditAttribution: yched commentedMight very well be so, but then this is about the model that is our current D8 state more than about the nomenclature we put on it ?
I mean, the model & pattern might be bad, but I'd think we're better off putting names that designate it clearly for what it is ?
Comment #8
msonnabaum CreditAttribution: msonnabaum commentedAgreed. Handler in no way designates it for what it clearly is. It's a way to not name a class properly by saying they are all in one role when they are not.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedI agree that handler isn't a perfect name, and that they're not all in one role. But one perspective I'd like to offer, feel free to disagree: access, list, form, and render are kind of the UI equivalent of CRUD (not individually a 1:1 map, just when taken as a whole). Whereas storage is the non-UI part of CRUD.
Comment #10
tstoecklerWith #1983844: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case this just got a whole lot more confusing. We now have an EntityListController class which is in fact not an entity controller, but a route controller, which finds the correct entity list controller for the entity type. For extra fun, it is located in \Drupal\Core\Entity\Controller\EntityListController while all the entity controllers are located directly in \Drupal\Core\Entity. Lucky that we don't have a general list controller base class otherwise we would have \Drupal\Core\Entity\Controller\EntityListController call a method of \Drupal\Core\Entity\EntityListController.
Comment #11
fagoI like the idea of doing away with the handler/controller suffix. Initially I thought it maps well to Storage and Form already, but not so with e.g. rendering. I think that is as we are used to referring to the form functions handling a form as "form", while we aren't used to something similar for Renderable.
$list->render() fits very well also, I'm not sure about Renderable and AccessController though. Maybe better we'd have AccessControll to differentiate better to Controllers, thus $access_controll->viewAccess() ?
For Renderable, why not go back to "View" ? We have entity forms and form modes, why not have entity views and view modes? "RenderController" is actually a bit un-specific, forms or lists are rendered also. What we are rendering here is an entity view though, so why not have $block_view->render() instead of $block_render_controller->view() ?
Comment #12
fagoLike a Form isn't the real form, but a "thing that builds the form array you can process". It still represents our form though.
Comment #13
tim.plunkettWe've already started ditching "Controller" in some form controllers, like \Drupal\filter\Form\FilterDisableForm and \Drupal\user\Form\UserRoleDelete
I agree that we should ditch "Controller" where it isn't needed to explain what it is:
But for the other ones, I say we swap for Handler, just to resolve any ambiguity with Controller (which is becoming more of a problem the further #1971384: [META] Convert page callbacks to controllers progresses):
Comment #14
msonnabaum CreditAttribution: msonnabaum commentedI could live with those, but FooAccessHandler is much better than the others imo.
Comment #15
Crell CreditAttribution: Crell commentedMark, you're not going to push for FooRepository instead of FooStorage? :-)
Comment #16
fago#13 works for me. The only thing I'm still bother with whether we should revert back to "view-controller" or "view-handler" instead of rendering, as
- not only the entity-view is rendered, the result of a form also - so that's unspecific
- methods do not involve rendering, actually rendering is done later on - so it's actually wrong
- "render" does not map with the hooks fired, which are "view"
So what about FooViewHandler?
It's entity-view and we already have a hook called like that, so if it's confusing with Views module it is already.
Comment #17
effulgentsia CreditAttribution: effulgentsia commented+1 to #13. How does it affect annotation keys?
+1 to #16 if whoever rolls this patch is willing to include it as part of the scope.
Comment #18
tim.plunkettRight, we still have the "controllers" top-level key in the annotation.
If we're switching Controller to Handler in most of them, we should rename that "handlers" as well.
Comment #19
msonnabaum CreditAttribution: msonnabaum commented@Crell - At this point, I think EntityManager is closer to a repository. It's a bit ambiguous atm.
Comment #20
xjmDiscussed with @Dries, @effulgentsia, @msonnabaum, and @alexpott. We came to the conclusion that renaming these is worth the BC break and disruption to other patches. We agree with the first part of #13. For render, we'd like to propose
FooRenderBuilder
, and for access and translation, let's try to come up with some other possibilities for the names. It would be better to avoid "handler" if possible.Let's re-scope this as a meta issue and create sub-issues for each name pattern. The first three name changes in #13 are approved and we can start creating patches for those right away.
Comment #21
xjmComment #22
BerdirSo, we had a lot of fun discussing this one, these are the conclusions we reached:
CONCLUSION: ones we have and how we will rename them:
- storage -> FooStorage
- form -> FooOperationForm
- list ->FooList
- view_builder -> FooViewBuilder
- access -> FooAccess
- translation -> FooTranslationHandler, FooTranslationUiAdaptor, FooTranslationUiProvider, FooTranslationUiBridge → conclusion is that we cannot name this as probably most of it will go away and we are not sure on what will be left here.
It’s fine if the patch only renamed the classes, because that’s a good DX win already. Then we open a separate issue for renaming the controllers key and moving certain keys to top level.
TODO: open issue to settle on the name of the parent key (previously controllers): handler vs service
Full notes: https://docs.google.com/document/d/108NNmlN2QmX_d3db00MQE9Cwbc_YsmubT4at...
Comment #23
BerdirAnd here's the first one: #2097903: Rename EntityRenderController to EntityViewBuilder
Turns out to be more work than I thought, so moving to sub-issues.
Comment #24
BerdirTagging.
Comment #25
andypostThe new one #2100467: Menu links are not ready to use a view builder so remove it from the annotation for now
Comment #26
webchickDo we still agree this is major, given that
20%13.4% of the criticals are entity/field API related, and patches like these will only force re-rolls of those, plus other bug fixes in the queue?Comment #27
amateescu CreditAttribution: amateescu commentedWasn't it ~13.4% three days ago? I, for one, still think this is major and that it needs to get done.
Comment #28
webchickYep, sorry. I forgot my calculations. :) I'll respond in the other issue.
Comment #29
andypostAdded #2154435: Rename EntityAccessController to EntityAccessControlHandler
suppose the biggest patches would be form and storage related
Is this issue still major? maybe better to postpone this one to allow other changes to make in?
Comment #30
fagoThat's a rather big change in how devs perceive the system and affects a lot of class names, thus imo it should be targeted for beta.
Comment #31
fagoSo according to #22 we've an agreement, except for:
We need to figure this out *now*, or we'll stay with totally confusing controllers.
Personally, I like "service" but I don't think it's a good match for all of the controllers as some of them expose no API that is meant for public consumation at all, e.g. stuff like the entity translation controllers or ViewIntegration controllers. Consequently, I think that "handler" fits us better.
Thoughts?
Comment #32
amateescu CreditAttribution: amateescu commentedI remember there was a discussion once to register these entity "controllers" in the DIC, and maybe that's something we still want to do. Would that influence the decision here?
In any case, I prefer "handlers" too.
Comment #33
andypost+1 to "handlers", also patches in issues needs to be updated to use this!
Comment #34
andyceo CreditAttribution: andyceo commentedAnother +1 for "handlers" :)
Comment #35
andypostLet's get conclusion here, any other thoughts?
Comment #36
effulgentsia CreditAttribution: effulgentsia commentedI'm still ok with 'handlers' as the top-level key for all of them.
Alternatively, per #31, we could introduce two top-level keys: 'services' and 'delegates', where the first is for ones for outside use and the second is for ones completely encapsulated by the entity class. However, while the naming purist part of me likes this alternative, I see two potential downsides to it from a practical standpoint:
1) We'd need to agree which are which, and I don't think that will get settled very quickly.
2) 'services' would be the correct term from a DDD standpoint, but could cause confusion due to the *.services.yml file being something similar but not exactly the same.
Comment #37
xjmComment #38
andyposts/EntityListController/ListBuilder suggested in #2120871-45: Rename EntityListController to EntityListBuilder but this list builder bound to be used for entities
The same question is about
EntityAccess
that actually access checker but this name already used, and EntityAccess is actually bad name #2154435: Rename EntityAccessController to EntityAccessControlHandlerComment #39
andypostFiled suggested issue about storage #2188613: Rename EntityStorageController to EntityStorage
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedI thought more about #36 and the concerns I raised there, and have an updated recommendation:
Thoughts?
Comment #41
tim.plunkett1) That feels like cheating, but doesn't bother me so much.
2) Unless we add a method to EntityManager for every single public method on each of the different controller interfaces, I don't see how this is feasible. And that would be a lot of methods.
3) We're actually really really close to not needing getFormController at all, only HtmlEntityFormController and EntityFormBuilder should be using it... I think getForm() is confusing in that sense since it returns an object, not a form array.
4) I really don't understand this one. Unless we use magical naming, how would we find the right service for a given entity type? And then we'll need FooServiceProvider classes everywhere?
Comment #42
fagoProblem with #40 is that we need this to be extensible on a modular basis.
E.g., Rules would provide something like a RulesIntegrationController/Handler/Service/something which integrates an entity type with Rules, but can be overridden and adapted as needed. Removing getController() because of naming issues seems like a step back as it removes the re-usability of that and would urge every module needing that to reinvent it.
Comment #43
andypostSO what is a fate of #2154435: Rename EntityAccessController to EntityAccessControlHandler - do we really wanna have 2 kind of controllers?
1) request handlers
2) access handlers
Also
EntityStorage
needs review in #2188613: Rename EntityStorageController to EntityStorageComment #44
xtfer CreditAttribution: xtfer commentedFWIW, I've just wasted an evening fixing a range of test config entities because their keys and classes were incorrect, and I feel like there is no real improvement or value in any of the keys or names chosen.
This does appear to me to be a bikeshed which has somehow resulted in renaming. If we can't think of good names for the types of objects we create, we probably shouldn't waste time redesigning their naming conventions and keys. For example, "list" to "list_builder" makes almost zero difference to someone who doesn't understand the intricacies of the system.
Just a thought...
Comment #45
BerdirAnd the entity form rename is up: #2238077: Rename EntityFormController to EntityForm
Comment #46
andypostSo only key name and last one #2154435: Rename EntityAccessController to EntityAccessControlHandler left here
Comment #47
mparker17Updated the issue summary.
Comment #48
mparker17On @xjm's advice, I've improved the API change "before" and "after" a bit.
Comment #49
xjm@berdir raised yesterday the question of the annotation group key itself. At this point I think it would be best to simply change 'controllers' to 'handlers'.
Comment #50
BerdirYep, that + methods like getControllerClass(), probably some documentation references to "entity controllers" or so, and $form_state['controller']: #2313931: Merge $form_state['controller'] with $form_state['build_info']['callback_object'] and add a method for it.
Comment #51
BerdirHere it goes :)
- Renames controllers to handlers
- Renames all *Controller* methods on EntityType and EntityManager to *Handler*
- Removes entity_get_controller()
- Updates a ton of sometimes insanely old and outdated documentation, beautiful things like this:
Comment #53
BerdirUps, somehow missed a few entity types and variables/documentation, let's see if this works better.
Comment #54
andypostneeds dot(.) at the end of sentence
let's unify them once touched
Comment #55
fagoGreat to see the renames finally become complete. The approach taken looks great to me and makes a lot of sense. I've not done a full, proper review (yet), just checked the patch from a bird-eye view.
Comment #58
BerdirRe-roll after some minor context conflicts due to renames and additions, tried to unify the documentation a bit more.
Comment #60
BerdirFailed because of the new views_data entity handler.
Comment #63
BerdirRe-roll.
Comment #64
tim.plunkettIt's hilarious to me that this function is still around.
Reviewed line by line, thanks @Berdir for seeing this through.
Comment #65
alexpottThe doc block for this needs a update and also EntityManager::$controllers. Additionally - $controller_class_getter? Maybe this should be changed to {@inheritdoc} and the class getter added to the interface.
In EntityApiInfoTest needs fixing too.
Comment #66
alexpottEntityManager::getFormObject() also uses $this->controllers
Comment #67
BerdirOk, cleaned up everything now I think. All remaining "controller" mentions in Drupal\Core\Entity now refer to actual controllers.
Comment #69
BerdirRe-roll.
Comment #70
dawehnerPS: This looks really sane. No real out of scope changes, berdir++
I love this change. From one pointless line to another one, but sure not a problem of this patch.
I wonder whether we could improve the exception message here. It should rather be "... dit not specify a %s handler".
I wonder whether it could be named in a more abstract way like, $handler_class_getter
I don't see a place though where this parameter is passed along.
Shouldn't we talk about the storage cache here?
Comment #72
BerdirDiscussed the class_getter stuff with @dawehner and @timplunkett and nobody quite figure why we need it exactly, so it is gone now.
2. Using handler now.
the second 2.: Yep, made it storage cache now.
Comment #74
BerdirSomeone is messing with me :)
Comment #76
BerdirFixed that fail, looks like we got a new test entity type in the family ;)
Comment #77
andypostLooks all addressed
Comment #78
plachI guess it makes sense to rename this issue now.
Comment #79
alexpottCommitted 629ebca and pushed to 8.0.x. Thanks!
Comment #81
yched CreditAttribution: yched commentedWoohooo ! Awesome !
1 year and a half for this rename :-)
Comment #82
BerdirUff, that was quite a ride :)
I updated https://www.drupal.org/node/2200867 slightly and re-published it, so that people get notified.