In addition to _content, _controller, and _form, the entity system provides _entity_form as an easy way to call EntityFormControllers from routes.

Recently in #1983844: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case (which hasn't even had a change notice yet!) we added \Drupal\Core\Entity\Controller\EntityListController to handle this for entity lists, but that was partly confusing since it called through to something usually extending \Drupal\Core\Entity\EntityListController, and it was more verbose than _entity_form.

Here's what it looks like in HEAD now:

user_role_list:
  pattern: '/admin/people/roles'
  defaults:
    _content: '\Drupal\Core\Entity\Controller\EntityListController::listing'
    entity_type: 'user_role'
  requirements:
    _permission: 'administer permissions'

And here it is with this proposal

user_role_list:
  pattern: '/admin/people/roles'
  defaults:
    _entity_list: 'user_role'
  requirements:
    _permission: 'administer permissions'

Eventually, once our page callbacks are converted, we'll likely want one for _entity_render as well (think node/%node, user/%user).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
9.52 KB

See attached.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.phpundefined
@@ -37,8 +37,13 @@ public function __construct(ContentNegotiation $negotiation) {
+      elseif (!empty($defaults['_entity_list'])) {
+        $defaults['_controller'] = '\Drupal\Core\Entity\HtmlEntityListController::content';
+      }

Can't this just set _content and let the HtmlPageController do what's needed? Everything else causes problems with anything else then full rendered pages.

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityListController.phpundefined
@@ -0,0 +1,42 @@
+    $content = $this->container->get('plugin.manager.entity')->getListController($_entity_list)->render();

Inject the entity manager!

tim.plunkett’s picture

I'm just following what HtmlEntityFormController does now, but that seems sane.

HtmlEntityListController mimics HtmlFormController and others by being container aware, I don't know that it can get anything else injected.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.45 KB
9.52 KB
7.21 KB

Let's just reuse the existing class, which is from my perspective already in a better place and shape in general.

dawehner’s picture

FileSize
7.46 KB
1.69 KB

This time it should actually work.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.phpundefined
@@ -37,8 +37,14 @@ public function __construct(ContentNegotiation $negotiation) {
+        $defaults['_controller'] = 'controller.page:content';
+        $defaults['_content'] = '\Drupal\Core\Entity\Controller\EntityListController::listing';

That's tricky! I like it.

I think I wrote too much of this to RTBC myself.

chx’s picture

    * Provides the listing page for any entity type.
    *
+   * @param string $_entity_list
+   *   The entity type to render.
-  public function listing($entity_type) {
+  public function listing($_entity_list) {

I think $entity_type is a better variable name for an entity type than $_entity_list -- that variable I would expect to contain a list.

dawehner’s picture

Cood point, let's avoid this uglyness if possible.

smiletrl’s picture

I think
+ * @param string $_entity_list
is trying to keep variable name consistency, because it's using this "_entity_list" in yml file
+ _entity_list: 'custom_block_type'
to match new routing system.

But yeah, it's ambiguous to me too, how about '"_entity_list_type'?:P

Here's another question, but it's non-related to this specifc issue, sorry for bugging you here:(

Will every route_enhancer.** class provided by core.services.yml will be invoked by the new routing system? For instance, this issue provides a _entity_list: for defaults at routing yml file, routing system will call every route_enhancer.** class based on this specific routing file to get a final "defaults" status for this route. Is this understanding right?

It seem there's no conflicts between all these route_enhancer classes. They check each defaults property respectively. For example, ContentControllerEnhancer is to check '_content' property for 'defaults', EntityFormEnhancer is to check '_entity_form' property. And the new route_enhancer EntityRouteEnhancer in this issue is to check defaults property '_entity_list'

So, other modules could provide its own routing-enhancer class to manipulate the routing system too?

dawehner’s picture

But yeah, it's ambiguous to me too, how about '"_entity_list_type'?:P

There is currently already _entity_form so let's be consistent with that.

Yes every route enhancer is fired on every request and you can implement your own if needed. In general I would rather avoid that and rely more on the configuration in yml files as it's easier to understand. These entity related functionality though is worth as there are so many implementations in core.

andypost’s picture

+1 to _entity_list also I think it's ok to name argument as 'entity_list'

damiankloip’s picture

This patch is looking great, just some questions about things more than feedback.

+++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.phpundefined
@@ -0,0 +1,54 @@
+   * Content negotiation library.

I guess this is future proofing it, atm it seems like we can't really call that a library :)

+++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.phpundefined
@@ -0,0 +1,54 @@
+    if (empty($defaults['_controller']) && $this->negotiation->getContentType($request) === 'html') {

So having a controller could always override this. Not saying it's wrong, is that what we want? could be.

+++ b/core/modules/views_ui/views_ui.routing.ymlundefined
@@ -1,7 +1,7 @@
+    _entity_list: 'view'

Stuff like this makes it all worth it!

Also, seems like this could do with a unit test? maybe I missed it somewhere else. I guess there are lots of tests that test the actual listing pages. I don't know.

dawehner’s picture

So having a controller could always override this. Not saying it's wrong, is that what we want? could be.

That's the common pattern we use in every route enhancer. It solves the problem to not let different route enhancers get into conflict.

Here is a unit test.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

one more DX win:)
one could say providers could be used for the test, but its not that consistent..so i am gonna rtbc this

andypost’s picture

+1 to rtbc

tim.plunkett’s picture

Adding two more converted listings.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, entity-list-2008356-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
413 bytes
12.16 KB

/me facepalms

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll

 curl https://drupal.org/files/entity-list-2008356-18.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 12448  100 12448    0     0   9241      0  0:00:01  0:00:01 --:--:-- 10871
error: core/modules/views_ui/lib/Drupal/views_ui/Routing/ViewsUIController.php: does not exist in index
error: patch failed: core/modules/views_ui/views_ui.routing.yml:1
error: core/modules/views_ui/views_ui.routing.yml: patch does not apply
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
12.17 KB

I was trying to delete
_controller: '\Drupal\views_ui\Routing\ViewsUIController::listing'

Now I'm deleting
_controller: '\Drupal\views_ui\Controller\ViewsUIController::listing'

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5bf89a4 and pushed to 8.x. Thanks!

tim.plunkett’s picture

Title: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form » Change notice: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form
Priority: Normal » Critical
Status: Fixed » Active

Needs a change notice.

dawehner’s picture

Status: Active » Needs review
andypost’s picture

Title: Change notice: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form » Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form
Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community

Awesome!

dawehner’s picture

Status: Reviewed & tested by the community » Fixed

Perfect

Automatically closed -- issue fixed for 2 weeks with no activity.