Part of meta-issue #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1
Problem/Motivation
This patch is being developed in the CMI sandbox: config-list-1781372-tim branch
One of the big things that Views needs CTools for is the "Export UI":
Obviously we have to remove that dependency for Views to go into core, but more than that, this would be useful for all configuration entities, e.g., the vocabulary listing at admin/structure/taxonomy
.
We've rewritten Views to use that already, and the above screenshot show the new code, not CTools :)
Proposed resolution
Introduce EntityListControllerInterface
and a list controller class
key for hook_entity_info()
.
Implementation
With the current patch (which removes non-essential functionality and defers it to the followup issues below), you can implement the list for a given my_entity
entity type provided by my_module
with the folllowing steps:
-
Define
$return['my_entity']['list controller class']
in themy_module_entity_info()
hook implementation.The class must implement
EntityListControllerInterface
. For atheme_table()
listing of entities similar to the Views screenshot at right or the currentadmin/structure/taxonomy
page, use or extendEntityListController
. For configuration entities, use or extendConfigEntityListController.
-
Create a page callback that calls
entity_list_controller('my_entity')
and renders the result. - Define a path for the listing in the
my_module_hook_menu()
. - (optional) Define a
MENU_LOCAL_ACTION
menu item with a page callback that adds entities of the given type.
See #1783964: Allow entity types to provide menu items for followup discussion on how the path definitions for CRUD operations might be simplified.
Followup questions
-
#20: The possible operations for an entity are actually bound to the entity, not the list [controller]. It would make much more sense to have that as a concept and method on the
Entity
class. The per-entity operations can be re-used in many other situations. See #1783964: Allow entity types to provide menu items. -
#20: We need to figure out how to dynamically enhance the output/operations for
BundleConfigEntity
objects; i.e., Configurables that represent bundles for another entity type (e.g.,NodeType
entities are the bundles forNode
). These entity types typically have additional "manage fields" and "manage display" links in their operations. The'fieldable'
property in entity info cannot be leveraged, because that property is not set for the entity we're listing, but instead is a property of the "other" entity type that ours provides bundles for. Since it becomes more and more clear that we'll need aBundleConfigEntityInterface
for these kind of entities, we could check the implemented interface though. -
The current config entity listing pages in HEAD only output the label of the Configurable, but not the machine name. The node/content type listing page is the only exception; it outputs
@label <small>(Machine name: @id)</small>
in the Label column. Exposing the machine name is definitely beneficial for users that are seeking for a particular one, but an entire (table) column looks and feels too dominant. -
The operations links (e.g.
edit
,delete
, etc.) are related to each individual entity and defined in the list controller class for rendering, but in response to feedback (#16), the listing path itself and the path to add a new entity of the given type (which are related to the entity type rather than specific entities) are set separately inhook_menu()
. As a result, the "Add" link is not displayed within the empty text of the listing table, because this text cannot be set throughhook_menu()
. #1783964: Allow entity types to provide menu items should provide a more complete solution. See #102.
Related issues
- #1783964: Allow entity types to provide menu items
- #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones
- #1797740: Introduce the EntityTypeAwareInterface interface for entity operations uniformisation
- #1798332: Add paging to the EntityListBuilder
- #1798540: Add link to add a new entity in an empty entity list controller table
- #1799600: Add test of sorting for configuration entities
- #1810350: Remove menu_* entity keys and replace them with entity links was postponed on this issue.
Comment | File | Size | Author |
---|---|---|---|
#113 | 1781372-config-list-113.patch | 20.58 KB | andypost |
#113 | 1781372-interdiff-111vs113.txt | 4.04 KB | andypost |
#114 | 1781372-config-list-114.patch | 18.71 KB | andypost |
#114 | 1781372-config-list-111vs114.txt | 1.24 KB | andypost |
#111 | config-1781372-111.patch | 18.75 KB | xjm |
Comments
Comment #1
tim.plunkettTagging.
Comment #3
tim.plunkettconfig-entity-list.patch queued for re-testing.
EDIT: I opened #1781398: Drupal\language\Tests\LanguageUILanguageNegotiationTest->testUILanguageNegotiation() for the (hopefully) random failure.
Comment #4
tim.plunkettLast tag from me, I swear.
Comment #5
tim.plunkettc0febd0 Type-hint with EntityInterface instead of ConfigEntityInterface.
acc2db1 Remove redundant reloading of the controller.
9c22a9a Simplify EntityListControllerBase::__construct()
Comment #6
damiankloip CreditAttribution: damiankloip commentedA few doc tweaks/additions from todo's etc.. I also changed:
Seems we might as well just use empty instead? Just incase someone wants to just set the key or something dumb. I don't think this is babysitting code too much.
Comment #7
Gábor HojtsyWorks pretty well! I'm using this now in http://drupal.org/sandbox/goba/1782014 to work out a very simple module for D8 to support layouts with a list of common regions that layouts can share. The edit/delete/add operations are not yet implemented, need to figure out the best practices for entity form controllers now, but the list controller works flawlessly.
One UX note, IMHO we should default to exposing the 'id' as 'Machine name' and the 'Actions' as 'Operations' in the table header. This is the terminology used by Views and most of the rest of Drupal as well :) It is pretty odd that the proposed patch uses ID and Actions as user facing table headers.
Comment #8
damiankloip CreditAttribution: damiankloip commentedOK, that seems like a good point! Glad to hear it is working for you already.
Comment #10
Gábor HojtsyThat is the sanest default, and it let me loose some custom code :) http://drupalcode.org/sandbox/goba/1782014.git/commitdiff/8e41b7cf6aa36a...
Comment #11
tim.plunkettUpdated for #1760786: Move entity system "back" into a Drupal\Core component
Comment #13
catchIf #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) or a similar patch lands before this, it'd be good to do one conversion of a real ConfigEntity object here. If not then I guess those issues should also be converting the listings? I assume we want to use this for the listings of all ConfigEntity objects (thanks xjm for the least offensive pluralization so far) in core and save about 12 custom listings that way which'd be lovely. Haven't reviewed the patch here yet.
Comment #14
Gábor Hojtsy(I also use this now for the Drupal 8 port of http://drupal.org/project/gridbuilder :).
Comment #15
Dave ReidI do not like from a DX aspect that we are defining hook_menu() items in the controller. Menu callbacks are tied to modules, so why are we creating more work and harder to find items by defining them there?
Comment #16
Dave ReidAs an owner of the module I would find doing this MUCH MUCH easier than having to manage hook_menu() in a plugin. I can easily see what menu path I define, makes it easy to extend it with local tasks and actions since they're all in the same place.
I also pointed out in IRC that if we go with a hook_menu() method then likely we need to patch whatever localize.drupal.org uses to scan for translations as having menu callbacks not in hook_menu() will mean that the strings will be untranslatable until we fix it.
Comment #17
Jose Reyero CreditAttribution: Jose Reyero commentedSorry to interrupt, but is it too late for doing lists of configurable things that are not entities?
See issues with Config Entities at the end of this thread, #1754246: Languages should be configuration entities
Comment #18
Gábor HojtsyOn hook_menu(), based on the two modules I ported today to Drupal 8, indeed the add/edit and delete items would still be in hook_menu because entity form controllers so far have procedural mappers to retrieve and use the proper entity form controller + the deletion forms are not even handled in the entity form controllers, so what you get is:
defined in hook_menu:
- add item: function that uses the entity form controller
- edit item: function that uses the entity form controller
- delete item: drupal_get_form with regular confirm form
defined via hookMenu in the list controller:
- list items: assembled based on data from the methods
Depending on where hook menu is heading I don't necessarily agree that moving stuff out of there is bad. Based on the extent of changes planned, localize.drupal.org will need to adapt either way I think.
My sandbox where I have the list controller and form controller and delete confirms combined for a simple config entity with a machine name and label is at http://drupalcode.org/sandbox/goba/1782014.git/tree - hopefully all using current D8 best practices.
Comment #19
tim.plunkettThe controller needs to know the path, which is why it's currently in hook_entity_info.
The listing path should be added to hook_menu by the config module, as it currently is. And therefore, the implementing class needs to override the title, callbacks, etc. It would be a huge WTF if you specify a path and a controller and there is no menu item added.
If we want to somehow restrict the ability to add further items in the controller we could, but it will limit functionality.
If that's true, I think we can forge on with this approach.
Also, this is CNR, that was a random failure.
Comment #20
sunI completely agree with @Dave Reid above. However, I also see that the current situation with regard to proper encapsulation and separation between router definitions and entity controllers is very unclear. E.g., the EntityFormController somehow manages to get around the problem (similar situation with having to specify
$form_state['redirect']
there), though I didn't have time to study how exactly it does that yet. Anyway, let's create a separate issue to discuss that.Aside from that hook_menu() binding, the current code also adds a hard dependency on config.module for the entity list controller abstraction, which we should avoid...[snip]
Alright, I reviewed this and found too many issues, so I created a new config-list-1781372-sun branch in the CMI sandbox based on the latest patch here. Everyone has git access to that sandbox already.
Fixed minor code style issues.
Moved EntityListController* into entity system.
Fixed ConfigEntityListTest does not verify exact ConfigTest entity type.
Removed ::hookMenu() and moved list controller factory into entity.inc.
Removed all Ajax functionality. (Later, guys. :))
Fixed bogus 'add' link in operations.
Removed getList() method, as it duplicates EntityStorageControllerInterface::load().
Re-ordered header/row/operations builder methods.
Renamed methods into buildHeader(), buildRow(), buildOperations(), and render().
Restored getList() as load() method.
Fixed getOperations() does not respect/use Entity::uri().
Fixed entity label should appear first in lists.
Improved maintainability of render array definitions.
Still many open issues with this:
- First and foremost, the possible operations for an entity are actually bound to the entity, not the list [controller]. I think it would make much more sense to have that as a concept and method on the Entity class. The per-entity operations can be re-used in many other situations.
- I dislike how the buildHeader() and buildRow() methods are enforcing the concept and notion of a table output onto the list controller. I think this should be simplified; e.g., by merging buildHeader() into render(), and possibly renaming buildRow() to something else. In turn, there'd be no notion of a table baked in.
- We likely want to re-introduce a ConfigEntityListController base class specialized for ConfigEntity objects. (A first common operation for them is the ConfigTestListController::load() override.)
- We need to figure out how to dynamically enhance the output/operations for BundleConfigEntity objects; i.e., Configurables that represent bundles for another entity type (e.g., NodeType entities are the bundles for Node). These entity types typically have additional "manage fields" and "manage display" links in their operations. I was playing with the idea of leveraging the
'fieldable'
property in entity info, but alas, that property is not set for the entity we're listing, but instead a property of the "other" entity type that ours provides bundles for. Since it becomes more and more clear that we'll need a BundleConfigEntityInterface for these kind of entities, we could check the implemented interface though.The interdiff is not much of a help, but attaching it anyway.
Comment #22
sunAdded weights to operations to allow them to be altered/enhanced.
Fixed missing empty data handling.
Fixed ConfigEntityListTest failures.
Replaced ConfigTest entity specific list controller with generic ConfigEntityListController.
Comment #23
sunLeverage native render arrays for operations instead of theme_link() specific data. (this gives built-in handling for stuff like #weight, #access, #ajax, etc for each operation)
Also, @tim.plunkett moved the original branch into CMI as config-list-1781372-tim, and I just rebased mine onto that (replacing my remote branch).
Comment #25
sund'oh, sorry, the git diff for the patch in #23 went completely wrong... ;)
Comment #26
tim.plunkettThere were a boatload of changes between #11 and #25, which completely break the Views implementations, as well as @Gábor Hojtsy's.
I'm going through the changes and updating the Views implementation, commit by commit, to see if everything still works, and to look for things to push back on.
If this weren't assigned to me, I'd assign it to myself now :)
Comment #26.0
sunUpdated issue summary.
Comment #26.1
sunUpdated issue summary.
Comment #27
sunI started a list of architectural discussion topics in the issue summary (essentially some items from #20), so we can keep track of them.
Also, @tim.plunkett really wants to have a discussion on the idea of hookMenu() methods in entity controllers, and I agree we should discuss that. However, if we start to introduce that, then we should do so on all entity controller levels; i.e., including the entity forms + edit page (EntityFormController), the primary entity view page (soon EntityRenderController), and with this also the list page (EntityListController). We definitely should not hold up this issue on that; that's why I deliberately removed those methods. Let's make sure to create an issue for that. (Leaving to @tim.plunkett, since he appears to be very passionate about the topic anyway already ;))
Comment #28
tim.plunkettI've added #1783964: Allow entity types to provide menu items for the follow-up about menu items.
----
I've gone through the changes sun made in his patch, and I was able to adapt the Views implementation to all of them except one:
http://drupalcode.org/sandbox/heyrocker/1145636.git/commitdiff/c0bf36d
This changes the operations links from a render array prepared for theme_links(), to just an array render arrays for each operation, with '#type' => 'link'.
We're currently using theme_links__ctools_dropbutton(), and are working towards #1608878: Add CTools dropbutton to core.
I've brought config-list-1781372-tim all the way up to one commit behind sun's.
---
Of the 4 items added to the issue summary, I agree that #1 and #2 should be done, and I'm going to work on those.
I think #3 is a follow-up issue, possibly also blocked on http://drupal.org/node/1782460.
#4 is fine, I think it was only added for now as an example.
Comment #29
tim.plunkettHere's a new patch, also with
5b2f210 Use entity_get_info() to provide a default controller instead of hardcoding it in entity_list_controller().
Comment #30
webchick#29: listing-1781372-29.patch queued for re-testing.
Comment #31
sunTo make EntityListController a default implementation, we need to:
1) Move the ::getOperations() method into Entity (so nothing is left to override in EntityListControllers by default).
2) Remove the Base suffix.
Regarding the left-out commit mentioned in #29, it's most probably best to detach the operations entirely from any kind of render element type. I think the reasoning provided for the commit makes sense though:
- we need to determine user access for each operation, and
- each operation should also have a weight assigned to it, so other modules can inject further operations in between others (e.g., Field UI adding manage fields/display before delete), etc.
Comment #32
tim.plunkettebbcba6 Remove the obsolete part of config_test_list_page().
1a9f20d Make EntityListControllerBase a default implementation instead of an abstract class.
c66a096 Move getOperations() from the list controller to the entity itself.
Comment #34
fagoYes, having modular extensibility for operations would be a big plus.
I'm not so sure about adding the operations to the entity class though. It should be possible to use the controller to provide listings at various different places in the admin UI, but having operations defined in the entity class makes them go back to the "main UI" always. As of this would be problematic as breadcrumbs and various context is derived from the menu system (not sure whether that's supposed to change?). But also, it makes access checking and keeping control easier if everything goes through *your* menu items.
Why not just leave them in the ListController? The implementation could still default to the same operations, so it still could be as used as is.
Comment #35
tim.plunkettThis moves the getOperations back to the list controller, this time to the new generic implementation.
The operations DO have weights assigned, and that works as is right now.
I also removed the hardcoded table ID.
Comment #36
tim.plunkettThe tests still currently belong to config.module, but I'm not going to move them now unless #1785974: Move ConfigEntity into a Core component lands first.
Assuming #35 passes tests, I think it adequately addresses @sun and @fago's concerns, and should be ready.
Comment #37
attiks CreditAttribution: attiks commentedGreat work all, I tried this, but I guess I'm missing something
Patch applies cleanly and works, I get a list of my config objects with an edit and a delete link, yeah!
I'm using the following code (which might be wrong):
The problem is that breakpoints_breakpoint_load receives a mix of arguments, but I have no idea why, so I had to change the load function to the following, to suppress all warnings, but if anyone know how to fix it, great!
function breakpoints_breakpoint_load($id) {
if (is_object($id)) {
return $id;
}
if (is_array($id)) {
return $id;
}
return entity_load('breakpoint', $id);
}
Comment #38
tim.plunkett@attiks, you don't specify any page arguments for half of those menu items. That's your problem. See #1783964: Allow entity types to provide menu items for the follow-up
Comment #38.0
tim.plunkett.
Comment #39
attiks CreditAttribution: attiks commented@tim thanks, I'll have a look.
Comment #40
Jelle_SShould be changed to
because right now the link for the empty table isn't right. For example, with our breakpoints module getPath() returns
admin/config/media/breakpoints/breakpoint
. With the current code, the link turns in toadmin/config/media/breakpoints/breakpoint/admin/config/media/breakpoints/breakpoint/add
. Using the l() function solves this issue and returnsadmin/config/media/breakpoints/breakpoint/add
as expected.Comment #41
Gábor HojtsyWrapping
$this->getPath() . '/add',
in a url() is in fact better for translatability, vs. moving the whole link out to an l(), because it keeps the text with the 'Add one' properly together.Comment #42
Jelle_SThis should do it then.
Comment #42.0
Jelle_Supdate issue summary
Comment #43
Lars Toomre CreditAttribution: Lars Toomre commentedWhen this is next re-rolled, the following documentation issues hopefully can be addressed too.
@see directives should go after @return directive in this docblock.
Are we missing a docblock for this constructor? I think so since we need a @param documenting what $entity_type is.
Need a complete docblock for this method.
Missing @param directive here.
Comment #44
Gábor HojtsyUpdated regions module for the latest patch at http://drupalcode.org/project/region.git/commitdiff/6e83304b660453820213... - let me loose the list controller which is great IMHO, although I needed to add the URI callback which might be useful for other things.
Comment #45
xjmExciting!
@timplunkett asked me to take a close look at the documentation in the patch. Incoming!
Architectural and functional questions
Interesting. This seems kind of odd to me and I had to read these twice before it made sense to me because the relationship between
EntityListController::load()
andConfigEntity
isn't really explicitly here in this class.Is there a reason that the list controller interface and/or base class doesn't have its own
sort()
method? (I'll look through the issue to see if this was discussed already).At the least, I'd add another paragraph to the docblock, something like: "ConfigEntity objects are sorted for listing according to their own sorting method" or something. But it seems like this will need to be overridden or copypastaed constantly.
Test coverage
Okay, this is kind of weird. Why are we asserting the text appears in both lower and sentence case? I have no idea from the code. At the least, a more specific inline comment might be good, though maybe an xpath assertion would be better. What elements are we actually checking for here?
I guess we're verifying that the operation links both appear and are functional. Are there separate tests for what's actually on the edit and delete pages?
Incomplete or confusing docs
I kinda don't understand this at all from the docs. Where does this exception get thrown and what kind of exception? I didn't see any
@throws
anywhere in the patch. And what's the "default operation"? Meow? Maybe some @see would help? Not sure. :)(Also the wrapping is a little off; one line is 81 chars.)
This docblock seems kinda weird because I'd think that
EntitiesOfThisType::load()
does that. It's loading them for listing them, right? And that's different from their nativeload()
because why? (This also ties in to my earlier question about sorting.)Need a docblock for the constructor here.
This guy needs a docblock, most especially since it doesn't seem to be on the interface?
This should also document the expected array structure. Edit: Looks like it's a sorted renderable array of
#links
in the base class implementation.The header row for what? What's a header string? (Looking at the implementation on the base class, it's actually an array of table header cell labels, keyed by column ID, used for the #header in a render array for an HTML table. Right?)
An array of what data? Each row of what?
These methods both need complete parameter documentation so that the API module can make sense of them.
What kind of fields? To use for this entity how?
Eh? This isn't a member variable, it's a method. So I think this docblock is wrong; it wants an
sorry@param
@return
. :)Maybe something less vague here? Looks to me like it tests that the entity list controller loads the correct list of entities.
"...and confirm that it contains the expected results," maybe?
Style nitpicks
I think we discussed this before in another patch, but let's be a little more explicit here about what the entity info is ("as returned by
entity_get_info()
" or whatever, and/or with an@see
)."Tests the listing..."
This is technnically supposed to start with a third-person verb; we can say "Defines the default list controller for ConfigEntity objects."
I do this all the time myself, but these are English sentences rather than code "sentences" and so should end in periods rather than semicolons. ;)
Like Lars said, this should go down at the bottom of the docblock.
I need to go home before it gets dark, but I'll fix up some of these things once I get home and then talk to @timplunkett about the rest once I've read over the whole issue.
Comment #46
xjmAhh, I figured out what is going on here when I opened up the file. This was copied from the docblock for
entity_form_controller()
, which is why it makes absolutely no sense. :)Comment #47
xjmCleaned up what I could and handing back to @tim.plunkett. I addressed all the style and docs questions I had in #45 except for many of the points about the various render methods on the interface. I think everything in there could be documented more clearly, but I see #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1 linked in the summary here so I'm going to wait for someone who understands the render process better than I do to tell me if the interface is coupled to the Render not-an-API. :)
My questions above under "Architectural and functional questions" and "Test coverage" are still open.
Comment #48
xjmOh, forgot to point this out in my previous comment. This is also an open question. :)
Comment #49
xjmOops, typo to fix here. "Tests that the entity list controller..."
Comment #50
pounardEach time I see procedural code into a class my eyes bleed:
Could be replaced by something such as:
Which would untangle almost completly this code of global state.
Even better it could be done in a setter instead, thus allowing to create list instances without the storage (for usage with the prototype pattern or if you fetch a new instance from a factory).
I think that operations (where 'edit' and 'delete' are hardcoded) could be derived from the storage or whatever else controller, or could be just derived from menu entries (I just saw another path that proposes to declare menu entries within the controller => then we can automatically derivate the operations and compute the access rights in live).
I think the instance doesn't need to carry the entity info, only the getPath() method uses it, it would be better if the path could be looked up from another source (storage controller?) instead, thus we wouldn't need to copy those huge array in here (I now that PHP does copy on write, but that's not a good reason to keep those array copies everywhere!).
Comment #51
catchI think #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1 is only linked here because I pointed out this issue should mean several custom theme functions get removed - i.e. if/when vocabularies get converted to ConfigEntity objects the custom theme function for vocabulary admin goes bye bye when you combine that issue with this one.
Comment #52
fagoI don't think load() belongs into the interface.
If we want our implementation to wrap load() into another method, that's fine. But for outside use, load() is something that would be called on the storage controller.
Comment #53
tim.plunkett#50 would be fine if ConfigStorageController::$entityType and ConfigStorageController::$entityInfo were public or had getters, but they don't. We could change that, of course.
#52, EntityListController::load() is just
return $this->storage->load();
, however it gives the entity an opportunity to sort the entities in a specific way.Otherwise, every place the entities were loaded, they'd need to be explicitly sorted.
Comment #54
pounard@#53 It would greatly reinforce object responsabilities and encapsulation if we could fix that IMHO. That's not a blocker, but if we can do this while the patch is being discussed it would be time saved.
Comment #55
xjmThis ties back into my question about sorting above in #45.
Comment #55.0
xjmUpdated issue summary.
Comment #56
xjmAdding my changes above to the sandbox. For convenience/clarity, here's an interdiff for @Jelle_S's patch in #42.
Comment #57
xjmWe should also add test coverage for the link paths. And lots of other test coverage too. :)
Comment #58
xjmSo for some reason I can't push to the sandbox right now, but here's an interdiff that should apply on top of the branch tip + #56.
Comment #59
tim.plunkettI've updated the branch with the last couple patches.
There is still other parts of xjm's review I haven't addressed, hopefully I'll have time tonight.
Comment #60
fagoYep, that's fine. However that's missing in the functions docs. Anyway, I questioned not the existence of the method but whether it should be part of the interface.
It looks to me that everything that really needs to be in the interface would be __construct() and render()? There is a lot of implementation specific stuff in there though - why do we need this in the interface?
E.g. this here does not sound like it's actually built for outside use?
Although - I'd see getOperations (and buildOperations()) being useful so one can fetch specific operation paths from the outside. But then it needs proper docs of the return array.
Comment #61
andypostWorking on with #1552396: Convert vocabularies into configuration I get troubles with getting a list of vocabularies because of sorting issue - config entities are always sorted by the name
Comment #62
xjm@andypost -- For vocabularies, we'd want to override the sorting to be based on vocab weight, I think. Vocab will want to either override
ConfigEntityBase::sort()
and/or the sort method onConfigEntityListController
once we convert it. (@tim.plunkett and I just talked about replacing theload()
method with asort()
method.)Also, agreed on all @fago's points in #60.
Comment #63
sunConfigEntityBase::sort() already sorts by weight (if there is a weight property) first and label second (natural language). It's a very smart sorting algorithm that I originally invented for admin_menu. It should work for 99% of all config entities, hence located on ConfigEntityBase.
I guess the true topic of the discussion on ::load() is the question of separation of concerns. If we'd strictly separate stuff, then we'd actually have to inject the list of loaded entities into the list controller, instead of loading it from within. But then again, the original intention of ::load() [at least the way I see it] was to possibly enhance it later on with pager functionality or the like. (OTOH, that reminds of EFQ...) /me shrugs
Comment #64
andypostSorting throws warning
uasort(): Array was modified by the user comparison function
so just prefixed with @ - as suggested in https://bugs.php.net/bug.php?id=50688Comment #65
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedCan we fix #60 as well for better encapsulation ?
Also, for the sake of testing, I would rather prefer to have to pass the EntityStorageControllerInterface by constructor rather than get it in the constructor. In the former case you can easily pass a mocked object, while in the second case, you have no way of overriding the entity_get_controller method, or you have to override the __construct method.
Comment #66
pounardAgree with #65, this particular controller should not use any procedural accessors.
Comment #67
damiankloip CreditAttribution: damiankloip commented#64 and #65: Guys, please feel free to write the code and provide a patch for this...
Comment #68
pounard@#67 The issue seems to be already handled by 2 or more developers, I won't do this at the expense of what are doing the others. I'm suggesting ideas, if they like it they will do it. If they don't like it, they won't accept my patches anyway.
Comment #69
Lars Toomre CreditAttribution: Lars Toomre commentedI am tring to get my head around all of the various comments in this issue to figure out how to proceed. From a high level, what appears still needs to be done is as follows:
As per #52 and #53, modify ConfigStorageController and their ilk with:
As per #53, #63, #64 and #65, modify EntityListController.php by deleting load() and adding:
Finally, I am lost about what methods belong in the EntityListControllerInterface. In #60, @fago suggests only __construct() and render() are needed, but getOperations() and buildOperations() would be useful.
Does the above pseudo code make sense to roll into a patch?
Comment #70
tim.plunkettI've moved non-essential methods out of the interface.
I also documented the return of getOperations(), as I agree it is useful.
Both DatabaseStorageController and ConfigStorageController call entity_get_info() in the constructor, we can debate that in a follow-up.
I did however change the constructor to take the EntityStorageControllerInterface, that's a good idea.
The more I think about EntityListControllerInterface::load(), the more I think it's a good idea and we should keep it.
As sun points out in #63, it's a great entry point for further manipulation of the list. I've added a comment to that effect.
Code pushed to the sandbox, but here's a patch and interdiff for those following along.
Comment #71
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @tim. We were working on this at the same time.
I am curious what you think about what I suggested instead of load(). In #69, I was suggeting to add entity_items to the cunstructor as well as a getter and setter. A new sort function then would operate on the protected entityItems property.
I think that what I suggested adds flexibility while maintaining what already is in the patch. Hence, one could create a listing of all config objects of 'foo*'. Does it that make sense?
Comment #72
tim.plunkett#69 doesn't make any sense to me. Maybe provide a patch I can try out?
Updating the code for the ConfigEntity move.
Comment #73
moshe weitzman CreditAttribution: moshe weitzman commentedGreat patch
+ $controllers = &drupal_static(__FUNCTION__, array());
. Please justify static caches with performance benchmarks or something a bit less analytical if you must. Why do we have to cache here? I think caching could be done further down in the call stack by whatever is slow. FYI, an non-compelling justification is "I static cache here because FOO has a static cache."Comment #74
tim.plunkettI have a static cache here because entity_get_controller() has a static cache :)
Hilariously enough, it was so much copy/paste that it wasn't even being used.
Comment #76
tim.plunkett#74: listing-1781372-74.patch queued for re-testing.
Comment #77
attiks CreditAttribution: attiks commentedNever mind, need coffee
I doesn't apply anymore :/Comment #78
webchickLayouts, Gridbuilder, Bunnypoint, etc. are all using this so tagging for Spark.
Comment #79
Dave ReidI'm uncomfortable with the fact that URI callbacks are required for entities that don't actually use URI callbacks as publicly-visible 'view' paths as evidenced by Gabor needing to implement URI callback for region entities. See #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones.
Comment #80
tim.plunkett@Dave Reid, that makes sense to me as a new policy/standard, but I think your issue (which I followed) is the best place to codify that change. This patch doesn't make URI callbacks any ore required than they are already.
@damiankloip said he'd be working on the tests more today, yay!
Comment #81
Dave ReidTim, if I'm looking at the patch correctly, it seems it would force configuration entities to have an URI callback in order to get the edit and delete links when most of them aren't something that is 'visible' and has a view callback. It's not really something we can undo if this gets in.
Comment #82
tim.plunkett@Dave Reid, well, we could solve it in #1783964: Allow entity types to provide menu items, or subclass EntityListController for "public/visible" entities and override getOperations().
Comment #83
Gábor HojtsyIndeed, for me the easiest way was to define a URI callback, but I could have defined my own list controlled and overriden the operations. It was comfier to provide a URI callback even though it might not be correct from a technical purity standpoint.
Comment #84
Dave ReidOk great I wanted to make sure we have a plan in place to prevent abuse of the URI callback and not further regress. This is complicated since the config_test entity type implements a non-view URI callback, but that will be handled separately, it's just that this issue exposes it.
Comment #85
Lars Toomre CreditAttribution: Lars Toomre commented@tim This untested patch is what I was trying to explain in #69.
It would allow one (with no sub-classing) to pick items 2, 4 and 6 from the default list and to present the list in 4, 6, and 2 order. We also could use this EntityListController implementation, for instance, to display all taxonomy terms starting with 'V' or items 11-20 (a la a pager). My thought that the function presenting the list would do something like:
$listing = new EntityList($entity_type, EntityStorageControllerInterface $storage);
(The default list of entites are are already loaded by constructor via the load() method.)
$items = $listing->getItems();
// Do something with list like filtering or cherry picking items 2, 4 and 6 resulting in $filtered_items.
$listing->setItems($filtered_items);
// Sort the list with uasort() with custom comparison function 'custom_sort'.
$listing->sort('custom_sort');
$listing->render();
My one thought to address @Dave Reid's concern is do we want to add a method setOperations(EntityInterface $entity) ?
Comment #86
Gábor Hojtsy@Lars Toomre: Looks to me like it would be better to focus this on the use case we built it for and then refactor as needed / wanted later once we can use the functionality, instead of leaving this lingering in the queue not solving the initial use case either.
Comment #87
damiankloip CreditAttribution: damiankloip commentedAssigning to me for the tests. I got a few done; should have a finished patch by tomorrow morning!
Comment #88
andypostWorking on #1552396-50: Convert vocabularies into configuration & #1588422: Convert contact categories to configuration system
I found that there's no implementation of
EntityStorageControllerInterface::loadByProperties()
which could be usefulAlso there's bug with sorting #1552396-60: Convert vocabularies into configuration https://bugs.php.net/bug.php?id=50688
Comment #89
Gábor HojtsyAs for the bug with sorting, as per the PHP bug report (https://bugs.php.net/bug.php?id=50688) this sounds like an error/exception down the line in the sorting function which might be related to your test data related to label(). That is really the only "action" that the sort function executes on the config entity. If there is no error down in the label(), it should not modify the array in any way. So not sure how could we fix it, calling label() with @ in this patch does not seem like a great solution. It seems like pretty much a nasty edge case.
Comment #90
Gábor HojtsyOne more data point: the reason I'm clearly thinking there is probably something wrong with your source data is that I'm using this controller in the Region module which has a pretty complete test suite (http://drupal.org/project/region) and all regions sharing the same weight, and therefore ordered by label, hitting the label() methods, but no issues experienced (in manual or automated testing). I have automated testing there to edit default shipped regions, add new ones, delete both types, etc. So I don't see a reason to hold back this patch on that.
Comment #91
pounardThere is something that bothers me with this patch. When I read it, I see that the
EntityListController
acts as a real controller in the MVC sense of the "controller" word: it reads data from the model controller by input arguments and dispatches the result into the theme layer. It is a specific business callback therefore doesn't really need theEntityListControllerInterface
interface.Something really important that we absolutely need are paging and filtering capacities. In order to achieve that properly, this should be natively handled by the
EntityListController
generic instance. While the storage controller cannot, I'm enclined to use a LimitIterator instead, and leaving the load() method overridable for adding better paging handling depending on specific implementations.Operations should be handled outside of buildHeader() and buildRow(), those can be operations agnostic (and thus does not depend on getPath() which then become optional).
#85's proposal is conceptually wrong: your class is already doing the controller (in the MVC sense of it) job, adding the
setItem()
method on top of it is like stacking multiple controllers onto each others, it feels wrong. We have a controller here, we should use directly the Request object for getting filters and/or paging information.I like the idea of this controller, and a lot of code in it is good, but I am tempted to adapt it and write something that would look like this (see attached file), which is more encapsuled, a bit more extensible, and provide primitive paging and filter capabilities following what can do the entity storage interface.
Comment #92
damiankloip CreditAttribution: damiankloip commentedOk, here is some revamped test coverage. This should cover most things now, I hope.
@tim.plunkett: I created a new branch, config-list-1781372-damian, from your branch.
Comment #93
pounardI opened #1797740: Introduce the EntityTypeAwareInterface interface for entity operations uniformisation that would help cleaning up a bit the list controller constructor and some bits of the internals.
Comment #94
BerdirComing a bit late to this, ignore me if something has already been discussed and I missed it.
I agree that a number of things could be refactored like the entity operation stuff but that can be tackled later. I like the idea of this.
The conflicts between content and config entities are a bit tricky here, the focus of this is for config entities but it's enabled by default for all entities. No idea what's going to happen for an entity type that doesn't properly support this, considering e.g. the missing paging support. Maybe not much right now, but I guess we're going to further extend this later on. Wondering if this could be an optional feature instead that needs to be declared. Modules that attempt to use this in generic way would just need to check if there is actually a controller returned...
It's also a pity that there is a default value for the list controller but every config entity (those that will actually use it) need to override it anyway.
The first setContent() calls seems to be unecessary here?
hook_entity_info() should be updated and these two option should be added to the documentation there, once the approach has been agreed on. What the todo in the issue summary says :)
Comment #95
xjmThanks @Berdir! Not sure how everyone missed that we didn't update the hook docks... duh!
And I agree about the extra
drupalSetContent()
; I actually almost asked for that to be changed on my first review.Fixing that now, and adding a bit more additional test coverage.
Comment #96
damiankloip CreditAttribution: damiankloip commentedDrupalSetContent should be removed, its a clear mistake. I'm not sure it was there before though. I think I added that I my last patch...
Comment #97
pounardI think paging should be added to this patch, without it sounds not usable for anything else than types with less than 20 entities. It shouldn't be that hard to implement? May be the entity storage needs some love to allow this more easily; Anyway I think that in order to have something generically useful, we need this. Add filtering to it and you have the UI everyone wants.
Comment #98
tim.plunkett#97, I opened #1798332: Add paging to the EntityListBuilder
#96, no worries, xjm is going to fix it :)
Comment #99
pounardOh nice, thanks!
Comment #100
xjmOh. My @todo is still in here and needs a followup.
Comment #100.0
xjmUpdated issue summary.
Comment #101
sunCan we restore this to 'operations']['data' => $operations and remove the too early drupal_render(), please?
Aside from that, I'd be happy to move forward here. That is not to say that this would be set in stone. Quite the contrary, I am confident that we will revisit each and every part of this code, tear it apart, rip it out, revamp, and remix it back in. Working on this revealed a couple of very interesting questions and challenges; I not only foresee us injecting a (fancy new) Route object into the list controller, but I also see a very high potential for an entirely new concept of Entity Operations (view/edit/delete/duplicate/enable/disable/etc, and yes, including view), not only for the UI purposes here, but also for typical Entity Links on regular view (buried into ENTITY_build_content() right now), but even more so defining the corresponding callbacks and quite potentially more metadata (» Entity Actions, anyone?). We should also investigate how to make the output more themeable, which inherently, most likely means "swappable", but not from a developer/entity system perspective but a themer perspective. All of that will be very exciting to work on!
Comment #102
xjmUpdated patch from the latest in this issue's branch in the CMI sandbox. Attached:
Adds more functional test coverage to confirm the user can interact with the list controller as expected to add, edit, and delete an entity. This should also provide coverage for the bug @Jelle_S found.
Adds the
hook_entity_info()
docs (thanks @Berdir).Removes the
getPath()
method per protracted IRC discussion with @tim.plunkett. Currently it's not really used. We can revisit this in #1783964: Allow entity types to provide menu items.Clarifies the documentation for the render helper methods in
EntityListController
.Removes the
'list path'
entity info key.In the recent version of the patch, which uses the
hook_menu()
implementation to define the list path, the'list path'
key was only a workaround to tell the controller what link path to use for an "Add" link inside the table when no entities are listed. This workaround was not very robust and kind of weird and confusing (seemed like the worst of both worlds), so after discussion with Tim I removed it in favor of a complete solution from #1783964: Allow entity types to provide menu items. The screenshots below illustrate the change. I'll file a followup shortly to add back the link for better UX once #1783964: Allow entity types to provide menu items is resolved.Finally, Tim also added a commit addressing sun's point in #101 that's rolled into the patch here.
List controller knows its path
Pure hook_menu() path definition (current)
Comment #103
xjmFollowup issue: #1798540: Add link to add a new entity in an empty entity list controller table
Comment #105
sunThanks!
Final remarks:
The @todo is sufficient, no URL needed.
OK with removing @add-url for now, but that's a strange coding style change. Please revert to the previous/regular.
I don't quite get why we need XPath queries here and elsewhere -- testing for the expected string contents in the columns should be sufficient. We're not testing #theme table here.
This is not testing list controller functionality. CRUD operations for ConfigTest entities are tested elsewhere already and should not be duplicated. It's not trivial to draw a line here, but let's make sure to remove all unnecessary assertions here.
Comment #106
xjmI disagree somewhat; the previous version is hard to scan and confusing. It makes more sense for each parameter to have its own line, the same way that array elements get their own line. This is related to the
<editorial>
silly, bikeshed-a-licious</editorial>
#1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines, but I don't think it's worth nitpicking when there's no standard and both patterns for wrapping the assertion are used elsewhere in core. However, I don't actually care, so I'll just put it on one line. :)To differentiate it from the text of the page title and/or confirmation messages.
I also disagree with this. This tests that the links lead to the correct, actual, functional entity forms (see #40 for a related bug) and follows the expected interaction of the user. I could replace the
drupalPost()
with API calls for the CRUD and then reload the list... but why, since we're already on those forms anyway?Comment #107
xjmAlso, the current format of the functional test coverage codifies that the user is by default returned to the list after CRUD operations from it. Which is also worthwhile IMO.
Comment #108
xjmAttached addresses #105.
Comment #109
chx CreditAttribution: chx commentedShort of dedicating ourselves to get VBO in when Views gets in, I can't really see how else this can happen so go for it.
Comment #110
xjmI went through the test by morning (well predawn) light and I suppose these two assertions in particular could be removed:
For the rest:
assertLink()
,assertResponse()
, andassertTitle()
together confirm that the operations links work and lead to the correct pages.Comment #111
xjmAttached removes the two assertions in #110, which hopefully should do a better job of addressing @sun's feedback. Should still be RTBC since this is really a quite minor change. :)
Comment #111.0
xjmUpdated issue summary.
Comment #111.1
xjmUpdated issue summary.
Comment #112
xjmI updated the issue summary to better reflect the current status of the issue.
Comment #112.0
xjmUpdated issue summary.
Comment #112.1
xjmUpdated issue summary.
Comment #112.2
xjmThere aren't actually any user interface changes from this patch. Conversions to it will introduce those.
Comment #112.3
xjmUpdated issue summary.
Comment #113
andypostPatch exposes a trouble with sorting, just adds a weight to form and test the order
EDIT: filed #1798944: Convert config_test entity forms to EntityFormController
EDIT2: Troubles with sorting explained in #1552396-72: Convert vocabularies into configuration
Comment #114
andypostI think we should not stop this patch and find a solution for sorting in follow-up
Patch with just minor clean-up
Comment #115
damiankloip CreditAttribution: damiankloip commentedThe only thing I don't like is that this can be used for any entity type. I like how this was originally formed, when we first created this; for listing config entities only.
Comment #116
chx CreditAttribution: chx commentedThat's a valid question. Why this doesn't live in the Config\Entity namespace and uses ConfigEntityInterface where typehinting is used? The scope of this patch is utterly non-useful for generic entities IMO because while we expect config entities to be few we equally expect content entities to be numerous, needing filters, needing pagers etc.
And maybe I was wrong, there's no form in there so if this page would be, you know, a View (!) then all we would need to do is to provide the operation links? Am I very crazy thinking this?
Comment #117
tim.plunkettWell, there is #1798332: Add paging to the EntityListBuilder.
I don't like the idea of purposefully restricting this to ConfigEntity.
Yes, a view would be better suited to this, by why force it? If we want to change the namespacing/typehinting later, we can.
Comment #118
andypostAlso we still have no implementation for
EntityStorageControllerInterface::loadByProperties()
for config entities I think it could be useful for lists with filters thats why we have commented out some tests in "vocabulary-config" issueComment #119
webchickI can't tell from the recent discussion... is this still RTBC? Or do we need to refactor it? Could further improvements be done in follow-ups?
Comment #120
damiankloip CreditAttribution: damiankloip commented@andypost, I don't think this is the issue for what the configstoragecontroller doesn't implement.
@webchick, I would say this is still RTBC. My concerns were about people abusing this for any entity (as it lives in entity module now and not config). The patch itself is good though.
Comment #121
sunYes, this looks ready to me, except for the added test lines by @andypost in #113:
The final assertion does not assert anything.
I'm not 100% sure, but I guess the intention was to
1) make the first entity use an ID starting with "a", but a label starting with "Z" and a weight of 2.
2) make the second entity use an ID starting with "z", but a label starting with "A" and a weight of 1.
3) assert that the first item in the list is the second entity, and the last list item is the first entity. This should actually assert the $entity, not properties within.
Comment #122
tim.plunkett@sun, thankfully that hunk is only in #113, and not #114.
#114 is the patch to commit.
Comment #123
andypostMy patch just points that if there's a items with a same weight then sorting fails
Comment #124
andypostI meant this place! this one uses uasort and this place has no test
Comment #125
webchickWell now, THAT is pretty damn sexy. :)
I reviewed this code and it seems really well documented and clear. It's also nice that there are a bevvy of contributed 8.x modules atm making use of it, so it seems to be pretty rock solid. I agree with andypost that the bug he uncovered in the sorting functionality is definitely worth a follow-up, but since it was not introduced by this patch, should not hold it up.
So.... Committed and pushed to 8.x! YEAH! This is AWESOME stuff, folks!
I believe this needs a change notice, so that people porting their modules know to use this new API. Marking to a critical task for that. If you could turn that around quickly, it would be much appreciated, so it doesn't block other peoples' D8 features.
Comment #125.0
webchickAdd 'add link' issue to the summary.
Comment #126
tim.plunkettI've posted a change notice here: http://drupal.org/node/1799642
Comment #127
plachLooks pretty clear, thanks. I guess it will need to be updated for the follow-up about pagers and stuff.
Moving this to the entity system queue, where it belongs.
Comment #128
plachComment #129
xjmIs there a followup issue for the sorting issue? I thought @andypost said he was going to post it but I couldn't find it. Can we link it here?
Comment #130
pcambraI think is linked in the summary: #1799600: Add test of sorting for configuration entities
Comment #131
YesCT CreditAttribution: YesCT commented#1810350: Remove menu_* entity keys and replace them with entity links was postponed on this issue. (just adding this info so they are linked. also updated issue summary to add this to related issues)
Comment #131.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #132
xjmWell, this issue is fixed, so...
Comment #133
Gábor HojtsyI'm trying to introduce weights (tabledrag) into my existing entity list controller at #1787942: Allow assigning layouts to pages (so that pages can have weights edited sensibly in the admin list instead of their own number weight widgets on their edit form which is mostly useless). I've implemented tabledrags plenty of times before but I'm pretty puzzled as to how would I best integrate with entity list controllers if at all. To introduce this, I need a form, not a simple list render, I need submission handling, etc, and need to separate the form rendering/theming from the form itself (which the entity list controller does not in itself allow). I'd love to get some "best practice" feedback as to how it should be approached. Currently it feels like I might need to abandon entity list controllers in favor of my own tabledrag listing implementation (possibly in classic PHP functions fashion). Thanks!
Comment #134
xjm@Gábor Hojtsy, that sounds like something that would be generally useful--maybe open a feature request?
Comment #135
andypostAnd this really needed for #599770: Clean up the contact forms listing UI: Allow to set the default category and weights on the listing page
So please post here this follow-up issue
Comment #136.0
(not verified) CreditAttribution: commentedUpdated issue summary. adding info that #1810350: Remove menu_* entity keys and replace them with entity links was postponed on this issue.