Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
From #1783964-9: Allow entity types to provide menu items:
We really need to rename entity controllers. "Controller" is "callable that the Kernel calls", basically formerly "page callback". Like "bundle" that's an inherited term from Symfony so we cannot really change it.
Before
/**
* @Plugin(
* id = "block",
* label = @Translation("Block"),
* module = "block",
* controller_class = "Drupal\block\BlockStorageController",
* access_controller_class = "Drupal\block\BlockAccessController",
* render_controller_class = "Drupal\block\BlockRenderController",
* list_controller_class = "Drupal\block\BlockListController",
* form_controller_class = {
* "default" = "Drupal\block\BlockFormController"
* }
* )
*/
After
/**
* @Plugin(
* id = "block",
* label = @Translation("Block"),
* module = "block",
* controllers = {
* "storage" = "Drupal\block\BlockStorageController",
* "access" = "Drupal\block\BlockAccessController",
* "render" = "Drupal\block\BlockRenderController",
* "list" = "Drupal\block\BlockListController",
* "form" = {
* "default" = "Drupal\block\BlockFormController"
* }
* }
* )
*/
Comment | File | Size | Author |
---|---|---|---|
#60 | drupal_1807042_60.patch | 1.67 KB | Xano |
#50 | controllers-1807042-50.patch | 65.79 KB | tim.plunkett |
#50 | interdiff.txt | 1.18 KB | tim.plunkett |
#49 | controllers-1807042-49-a.patch | 65.67 KB | tim.plunkett |
#49 | controllers-1807042-49-b.patch | 65.92 KB | tim.plunkett |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedFWIW, I think Java calls these things "servants": http://en.wikipedia.org/wiki/Design_pattern_Servant.
Comment #2
plachWhat about Handlers? I think i'ts more intuitive.
Comment #3
sunI was a bit surprised by that comment and suggestion in the first place.
A "controller" virtually is a universal term, so I don't really get how and why we'd suddenly assume that "it means only one and this only one thing."
Comment #4
tim.plunkettHandlers are a Views term.
Let's call them context entity nouns.
Comment #5
Crell CreditAttribution: Crell commentedRelated: #1380720: Rename entity "bundle" to "subtype" in the UI text and help
Comment #6
plach@sun:
Same for handler, which is a far less characterized term than controller, I'd say.
It's used also in the Field API, btw.
http://en.wikipedia.org/wiki/Handler#Computing
http://en.wikipedia.org/wiki/Controller
Also manager could be fine but we are using it for DI, right?
Comment #7
Crell CreditAttribution: Crell commentedI don't think "Manager" has a fixed definition yet. :-) (yay, an unused noun!)
Comment #8
tim.plunkettPlugin managers.
Comment #9
Crell CreditAttribution: Crell commentedSo Plugin Managers manage plugins, Entity Managers manage Entities. Neither one has the overlap that "Controller" does, which has meaning outside of Drupal we're fighting against.
Comment #10
tim.plunkettSo we'd have a 'entity list manager', 'entity storage manager', 'entity form manager', and soon 'entity render manager'?
That's not too awful, if we actually decide we want to even bother :)
Comment #11
plachI'd still prefer:
'entity list handler', 'entity storage handler', 'entity form handler', 'entity render handler', 'entity translation handler'
but I can live with the manager terminology :)
Comment #12
sunHandler linguistically makes more sense than Manager, given the purpose of these classes.
Comment #13
Crell CreditAttribution: Crell commentedI am OK with handler if the Views folks are OK with it. It's less of a global conflict than controller.
Comment #14
dawehnerViews itself uses more and more the terminology of $handler_type plugins like views field, filter plugin
so i don't think that would really confuse people.
Comment #15
effulgentsia CreditAttribution: effulgentsia commented+1 to handlers. I'll be optimistic and call the bikeshedding portion of this issue over :)
Comment #16
tim.plunkettHere's a patch.
In code, the storage controller is just 'controller class'. In some comments that is also true, in others it is 'storage controller'.
That was fine when it was the only controller, but now that we have list and form, do we want to specify?
And if so, is that a follow-up, or just something we should do once so that anyone tracking HEAD doesn't have to rename it twice?
A second idea, why not "list handler", "storage handler" instead of "list handler class"?
Comment #17
tim.plunkettNote, this only changes the 'controller class' part to 'handler class'. Documentation and other places (entity_get_controller) still need changes.
Comment #18
tim.plunkettWe have consensus, which is what is important.
This patch already touches a lot of code, and renaming entity_*_controller would make it even more painful.
I'd postpone this, but since it's not major, there's no reason to.
I'm just not going to work on this until Decemeber 2nd. :)
Comment #19
Crell CreditAttribution: Crell commentedFor the record, I'm fine with letting this sit until Slush as long as Dratch doesn't object.
Comment #20
sunAgreed - this is super-typical clean-up material. :)
Comment #21
andypost-handler class?
+storage class!
Comment #22
fagoMaybe "service" would be a good term to use? I assume that in future those controllers will live in the DIC anyway:
- entity.storage.TYPE
- entity.form.TYPE
..?
Thus, you define the class for the entity storage service, or the entity form service?
Agreed, that this should be handled post-feature freeze.
Comment #23
fubhy CreditAttribution: fubhy commentedRelated: #1821662: Register entity controllers with the DIC
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedUnpostponing, because there's a couple issues I'd like to get to related to this, like #1831264: Use the Entity manager to create new controller instances, that I'd rather just do once with the correct names.
"needs work" because #16 doesn't apply any more. I'm willing to reroll, but what do you folks think of:
Notice the word "class" doesn't appear anywhere above, both for brevity, and so that if in the future we want to support the values being either class names or service ids, then we can do so (it's not hard for PHP to find out if a string is a class name or service id).
Comment #25
tim.plunkett+1 to #24
Comment #26
plachSounds reasonable to me too, but perhaps I'd add a " handler" suffix to make those keys more explicit.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedHere's step 1: renaming the keys according to #24, but not the classes or entity.inc functions yet. Wanting a bot check on just this much.
My suggestion is for those keys to be grouped inside of a top-level 'handlers' key. Do you still think they warrant suffixes too?
Comment #28
plachNope, I missed that part, looks pretty good this way :)
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedAnd here's one with all the classes renamed too.
Comment #32
tim.plunkettWOW!
This is great. I love the new annotation syntax.
Only missed two bits, as far as I see.
Comment #34
tim.plunkettBecause all of the renames are rather tedious, here's a patch that does just the changes to the annotation structure, without the actual change from handlers to controllers.
I'd like to see this get in first, and the renames can be handled in a separate patch.
This is a DX win on its own.
Comment #36
dawehnerDon't bikeshed about the name "controllers" as it will be replaced later anyway.
Talked with tim about that, we will probably have to rewrap the comments in one of the next paths anyway.
Annotation exception?
Just that line is a huge DX improvement!
Comment #37
tim.plunkettThanks @dawehner!
Fixed the trailing commas and the typo.
Comment #38
dawehnerAwesome.
Comment #39
sunThat makes a really great first step and looks really awesome! Thanks, @tim.plunkett!
Not even sure whether we need to rename from controller to handler. The original idea seems to follow the same paradigm as the idea for renaming blocks, but that was essentially won't fixed.
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedRetitling to aid committers. See #24 for what it will look like when the next part is done.
We currently have Drupal\system\CronController, which is a controller in the Symfony sense. But the Symfony controller for User module is named Drupal\user\UserRouteController to disambiguate from all the other nothing-to-do-with-routing entity controllers in the same namespace. I think renaming the entity ones to handlers still makes sense.
Comment #41
sunI think we're shooting ourselves in the foot with trying to disambiguate any terminology.
IMHO, the proper answer is namespaces:
If you provide a controller for entities, why don't you tell me so? Drupal\foo\Entity\FooController.
If you provide a controller for routing, why don't you tell me so? Drupal\foo\Routing\FooController.
Comment #42
Crell CreditAttribution: Crell commentedThe Symfony bundle convention is BundleName\Controllers\ThingieController.
It's just a code ambiguity question, though. It's also just normal conversation. "Do a thingie and add a Controller and modify the framistan". Which controller? Anyone who doesn't already know Entity API is going to think of the "thing formerly called page callbacks", and then when they see something else is also called a "controller" is going to go "WTF?"
Comment #43
webchickSomething I committed tonight conflicts with this.
I really love this patch quite a lot. However, since it's purely API clean-up, and also touches every single entity which provides a high likelihood of conflicting with feature patches, I'd rather wait until after Feb 18 to commit this, I think.
Comment #44
yched CreditAttribution: yched commentedOf interest to the folks subscribed here :
#1920498: Have rdf.module only act on renderable entities needs the EntityManager to be able to answer "does this entity type have a 'foo' controller ?" without a) actually instantiating the controller b) raising an exception as a means to say "the answer is no".
Comment #45
tim.plunkettConflicted with #1882240: Remove default assignment of render_controller_class in EntityManager among others, just keeping it green.
Comment #47
tim.plunkettWhoops, forgot to pull before rebasing.
Comment #49
tim.plunkettRerolled. It's after February (and March and April) 18th now!
Trying something out in patch A, but patch B should work fine.
Comment #50
tim.plunkettOkay, that confirmed my thoughts. Here we are with the most minimal annotation we could hope for.
Interdiff against 49-b
Comment #51
msonnabaum CreditAttribution: msonnabaum commented#50 looks great to me. Big improvement.
Comment #52
fagoThis looks like a great step forward. Just one thing though:
Where should modules add their controllers to then?
Comment #53
msonnabaum CreditAttribution: msonnabaum commentedOh, btw, I started a new issue for the controller renaming since this thread is now focussed on annotations: #1976158: Rename entity storage/list/form/render "controllers" to handlers
Comment #54
tim.plunkett@fago, http://api.drupal.org/api/search/8/hook_entity_info has been repurposed to act as a "build+enhance" pass to entity info gathering. It runs before alter.
That is where translation_entity should be hooking in.
Also, note that this @todo is only moved by this patch, it was added in the @EntityTYpe issue.
Comment #55
alexpottCommitted bc3a902 and pushed to 8.x. Thanks!
We need to look at the current change notices and update accordingly...
Comment #56
plachWe also need a change notice announcing this, I guess.
Comment #57
xjmComment #58
xjmThe title of this issue is misleading; can we retitle like this?
Comment #59
XanoThe exceptions that are thrown by the entity manager can now be quite confusion to people who've never seen them before, as a missing storage controller exception reads "The entity (...) did not specify a storage" (the word "controller" is no longer there).
Comment #60
XanoComment #61
alexpott@Xano can you create a new issue for #60... thanks!
Comment #62
XanoSee #1980464: EntityManager::getControllerClass() throws confusing exceptions.
Comment #63
tim.plunkettSince this is a HEAD2HEAD change, it doesn't need a stand-alone change notice, but I've gone through pre-existing ones and updated them:
http://drupal.org/node/1827470
http://drupal.org/node/1819308
http://drupal.org/node/1799642
http://drupal.org/node/1818734
http://drupal.org/node/1734556
Comment #64.0
(not verified) CreditAttribution: commentedAdding proposal