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.
Next part of #1976158: Rename entity storage/list/form/render "controllers" to handlers.
Not completely sure about everything in here, e.g. the Returns descriptions... seems like we need a new word for controller now...
Sandbox D8 Field API branch 2120871-entity-list-builder
Comment | File | Size | Author |
---|---|---|---|
#87 | 2120871-entity-list-builder-87.patch | 94.07 KB | Berdir |
Comments
Comment #1
BerdirHere's the patch.
Comment #3
BerdirThis should fix the test fails. Broken phpunit test results are broken :(
Comment #4
Berdir3: entity-list-controller-2120871-3.patch queued for re-testing.
Comment #6
BerdirRe-roll.
Comment #8
Berdir6: entity-list-controller-2120871-6.patch queued for re-testing.
Comment #9
dawehnerAny reason we don't convert it to "Contains ... directly"
We could add the "\" prefix right now.
I wonder whether we should name it DraggableListBase given that it is a abstract class.
{@inheritdoc}
{@inheritdoc}
This change is wrong, given that we do talk about the entity list controller (in the routing sense).
Comment #10
vladan.me CreditAttribution: vladan.me commentedRe-roll and updated according to #9
Comment #11
BerdirAnother re-roll.
Comment #12
Berdir11: entity-list-controller-2120871-11.patch queued for re-testing.
Comment #14
BerdirRe-rolled, updated for config translation, user and node list controllers.
Comment #16
BerdirFixing the interface.
Comment #18
longwave16: entity-list-controller-2120871-16.patch queued for re-testing.
Comment #20
dawehner16: entity-list-controller-2120871-16.patch queued for re-testing.
Comment #21
Berdir16: entity-list-controller-2120871-16.patch queued for re-testing.
Comment #22
amateescu CreditAttribution: amateescu commented:)
I suppose this wasn't intended.
Comment #23
amateescu CreditAttribution: amateescu commentedIt's not worth keeping up this patch any longer for those minor issues, I think it's RTBC.
Comment #24
amateescu CreditAttribution: amateescu commentedI meant this.
Comment #25
andypostThe config translation controllers actually replaces list controllers so should be renamed as well
Block module controller should not be affected
Also patch cleans up some remains
Comment #27
andypostFixed
UserList
namespaceComment #28
dawehnerDo we need to approve this API change?
This change is kind of out of scope ...
I wonder whether we should just point to the interface?
Comment #29
amateescu CreditAttribution: amateescu commentedThe parent meta (#1976158: Rename entity storage/list/form/render "controllers" to handlers) is already an approved API change.
Comment #30
andypost1) my bad, fixed
2) suppose no, api.d.o will show this links on implementation pages so it easy to navigate
Comment #32
andypost30: 2120871-entity-list-30.patch queued for re-testing.
Comment #33
andypostre-roll
Comment #34
xjmComment #35
Berdir33: 2120871-entity-list-33.patch queued for re-testing.
Comment #38
andypostre-roll
Comment #39
andypostand new places
Comment #42
andypostfix broken test
Comment #44
andypostAnd search page entity
Comment #45
BerdirThanks for the re-rolls.
This still makes no sense at all, this is not a list, this is something that creates/builds a list.
Just like EntityAccess isn't an access, it is something that checks access.
-1 to not having a suffix/name for this thing. What about ListBuilder, matches well with ViewBuilder?
Comment #46
XanoWithout knowing I suggested pretty much the same thing on IRC, so +1 from me.
Comment #47
andypostThis listing builder is highly coupled to entity so prefix removal will confuse but maybe it makes sense to file new issue to decouple
ListBuilderInterface
and inheritEntitlyListBuilder
fromComment #48
andypostI think better name for interface is EntityListBuilderInterface but implementations should have shorter names
Comment #49
andypostBetter doc-blocks
Comment #50
tim.plunkettThis is still wrong.
This is a ListBuilder, not a list.
Comment #51
andypostRe-roll patch to use Builder to replace Controller, just a questionable thing a config_translation controllers but they inherits from builder so changed too.
#50.1 This fixes wrong reference, so change is right.
Comment #52
andypostA bit of clean-up for docs
Comment #56
andypostFix broken tests and unify doc-blocks
Comment #57
andypostre-roll
Comment #58
Berdir#1981858: Rename hook_entity_info/alter() to hook_entity_type_build/alter() will rename that function, let's make sure to update this when this or the other issue is committed.
Patch looks RTBC to me, but since ListBuilder was suggested by me, I'd like another sign off or two on that. @timplunkett maybe? and @yched/@fago?
Comment #59
tim.plunkettLooks good to me!
Comment #60
alexpott2120871-entity-list-builder-57.patch no longer applies.
Comment #61
XanoRe-roll.
Comment #62
XanoComment #65
andypostOne more place to fix now
Comment #66
amateescu CreditAttribution: amateescu commentedBack to RTBC then.
Comment #67
xjmDraft change records are now required before commit. We don't do change records for every 8.x to 8.x BC break, but I think this change is big enough to require its own (very brief) change record for modules already using EntityListController.
The following two change records will also need very minor updates:
https://drupal.org/node/2089283
https://drupal.org/node/1799642
Once the change records are ready to go this can be set back to RTBC.
Comment #68
BerdirI don't think we have a change notice already for the ViewBuilder (I just checked for existing ones and fixed a bunch of references, including some other fixes), do we want to do separate change notices for them or is one enough? Or add it to one of the existing large entity field api change notices, we already have multiple of those...
Comment #69
xjm@berdir, can you link the issues and change records you're referring to? And maybe we can continue the discussion in the relevant other issue so we're not blocking this?
I think this issue only needs a very brief change record, like "EntiyListController renamed to EntityListBuilder" and two lines of text, just so there's a notification to the RSS feed and etc. 8.x to 8.x change records should probably usually follow that pattern.
Comment #70
BerdirDiscussed this a bit with @xjm, here's what we said would make sense:
- Start a new change notice "Entity controllers renamed"
- Reference the meta issue and all sub-issues (including this one)
- Describe the general concept of moving a way from the term "controller" for this, and using handler instead
- Also describe that different terms/suffixes are used for the different handlers, where appropiate
- Add a list of before/after class names (interface and base class, maybe an actual example like node) the annotation key and methods on EntityManagerInterface/EntityTypeInterface. Listing this all, this might be too much for a table, so maybe just add a heading ("EntityRenderController renamed to EntityViewBuilder") with descriptions below instead.
- Add a bold Note/Disclaimer that this is work in progress and not all renames have been executed. Maybe split the table in Implemented and Proposed renames?
- Include a complete example of before/after based on the current proposals. That should also help to understand the full picture.
I wanted to do this already but didn't, so if someone wants to start, please do so.
Comment #71
andypostre-roll after #2119905: Provide @ConfigEntityType and @ContentEntityType to have better defaults
Comment #72
andypostAdded stub change record https://drupal.org/node/2200867 - the changes of that issue already there
https://drupal.org/node/2089283 & https://drupal.org/node/1799642 - should be updated after commit
Comment #73
andypostre-roll after #2095303: Rename 'field_entity' to 'field_config' and 'field_instance' to 'field_instance_config'
Comment #74
BerdirWorked a bit on https://drupal.org/node/2200867.
It's not perfect yet (missing the other handlers, no example, as suggested in #70) but it should be good enough to unblock this issue.
However, I was wondering if we should also rename the list entity key to "list_builder", as we renamed "render" to "view_builder" and not just view. Right now, it's a bit inconsistent...
Comment #75
BerdirRe-roll. Only serious conflict was in entity_list_controller(), which was not yet renamed, unused and deprecated, so I just dropped it. Other than that, just two conflicts on use statements.
Comment #76
BerdirOk, @timplunkett also said list_builder, so I renamed that and also the ListClass methods. Let's see if I got that right.
Comment #78
BerdirMissed some setListClass() calls.
Comment #80
andypostFix for last broken test
Comment #81
Berdir80: 2120871-entity-list-builder-80.patch queued for re-testing.
Comment #82
BerdirRe-rolled, only conflicts where in Migrate and due to picture => responsive_image rename, and git took care of most of that, so no useful interdiff.
Comment #83
andypostJust a nitpick
Comment #84
andypostAdded sandbox link
Comment #85
tim.plunkettI like the added docs. Everything else is a 1:1 change.
Comment #87
BerdirLooks like andypost already pushed a merged version to the sandbox, this is the patch for that.
Comment #89
Berdir87: 2120871-entity-list-builder-87.patch queued for re-testing.
Comment #90
BerdirSegmentation fault in NodeTranslationUiTest :(
Comment #91
andypostyep, beck to rtbc
Comment #92
alexpottCommitted 5abaa5f and pushed to 8.x. Thanks!