Posted by xjm

This issue is a followup for #1781372: Add an API for listing (configuration) entities and #1783964: Allow entity types to provide menu items.

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

Current behavior

list-controller-no-add-link.jpg

Intended behavior

list-controller-add-link.jpg

Files: 
CommentFileSizeAuthor
#49 drupal_1798540_48.patch14.91 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,922 pass(es).
[ View ]
#14 drupal-new-entity-link-1798540-13.patch3.07 KBjamesquinton
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#13 drupal-new-entity-link-1798540-13.patch5.09 MBjamesquinton
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#4 drupal-1798540-4.patch3.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1798540-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

xjm’s picture

Issue tags:+Usability
xjm’s picture

Issue tags:+Entity system, +VDC, +Configurables
xjm’s picture

Title:Add link to add a new entity inside the entity list controller table when no items are listed» Add link to add a new entity in an empty entity list controller table
xjm’s picture

Issue summary:View changes

Updated issue summary.

dawehner’s picture

Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new3.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1798540-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I'm wondering whether this should be part of the interface, so i started without it.

It feels ugly to remove all existing config entities in the test to be able to check this
functionality.

sun’s picture

Hrm. We really need an entity operations/actions API. That has become crystal clear through a dozen of issues already.

I rather object to working around that missing puzzle piece even further. Let's fix the root cause/limitation instead.

An entity operations/actions API would inherently depend on #1696660: Add an entity access API for single entity access, since that introduces generic access handling, so each operation/action can be properly checked for access before it is exposed.

andypost’s picture

Closely related #1839516: Introduce entity operation providers

The "Add item/sub-item" could be a operations

sun’s picture

Issue tags:+Blocks-Layouts

The proposed change here attempts to hardcode a 1:1 relationship between an entity and where it appears in a listing, or perhaps rather, where its "Add form" is located.

However, architecturally, that's really not where we want to go. In case you know http://drupal.org/project/context_admin, that's much more in line with our architectural goals — entities can be listed in various contexts, and within each context, you are able to add a new, but also manage existing.

In a bit more abstract RESTful speak, this means:

List: GET /entity
List: GET /admin/content/entity

Add:  POST /entity
Add:  POST /admin/content/entity

Edit: POST /entity/{id}
Edit: POST /admin/content/entity/{id}
...

Unfortunately, the pure RESTful pattern cannot be applied to web-based form interaction. A typical way to work around it would be to replace the {id} with a static placeholder string that would be invalid as a regular/true ID otherwise; e.g.:

Add:  GET|POST /entity/:new:
Add:  GET|POST /admin/content/entity/:new:

In turn, regardless of the current context, you're able to retrieve the URI for creating/adding a new entity based on its regular URI pattern:

$entity = entity_create('foo', array(
  'id' => ':new:',
);
$uri = $entity->uri();

I'm aware that we didn't make much progress on contexts yet, but the proposed change here with a hard 1:1 binding would implicitly move us into a direction that is not aware of contexts, whereas we want to go into the opposite direction and make this stuff context-aware.

Tagging with Blocks/Layouts, in the hope that @EclipseGc and others might be able to chime in.

jibran’s picture

Status:Needs review» Needs work
Issue tags:+Usability, +Needs tests, +Entity system, +VDC, +Blocks-Layouts, +Configurables

The last submitted patch, drupal-1798540-4.patch, failed testing.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -64,6 +64,23 @@ public function load() {
+    if (isset($this->entityInfo['uri_prefix']) && isset($this->entityInfo['label'])) {

I think all entities should have uri() method implemented or use parent

dawehner’s picture

Well, but how can we use uri() to create a link to a creation for of entities? I don't see how this can workout.

dawehner’s picture

Issue summary:View changes

Typo.

damiankloip’s picture

Yeah, uri() won't really help us as we don't have an entity. Also I thought #1970360: Entities should define URI templates and standard links may have helped us here, but it seems that the summary mentions create urls but it didn't make it into the patch?

damiankloip’s picture

Issue summary:View changes

Removing myself from the author field so I can unfollow. --xjm

jamesquinton’s picture

Assigned:Unassigned» jamesquinton
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new5.09 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Re-rolled patch and updated.

jamesquinton’s picture

StatusFileSize
new3.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new3.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,919 pass(es).
[ View ]

Woops..! Here's the correct patch.

The last submitted patch, 13: drupal-new-entity-link-1798540-13.patch, failed testing.

The last submitted patch, 14: drupal-new-entity-link-1798540-13.patch, failed testing.

jamesquinton’s picture

StatusFileSize
new7.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,822 pass(es).
[ View ]

Updated and added link to the following lists:

  • Date formats
  • Custom block types
  • Custom blocks
  • Node
  • Image styles
  • Responsive image mappings

I don't think every entity list needs this, for example User Roles - where you can’t remove the default roles, and therefore never have an empty list.

“entity-add-form” link key could probably be named better.

jamesquinton’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Doh - rerolling without my .gitignore in the patch...

jamesquinton’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new7.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,740 pass(es).
[ View ]

Rerolled

dawehner’s picture

Thank you!

  1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -202,6 +203,24 @@ public function render() {
    +   *
    +   * @return string Add link

    A few more words would be cool

  2. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -202,6 +203,24 @@ public function render() {
    +      $link_object = new Url($link_template);
    ...
    +        $link_object

    Let's call it just url or url_object.

jamesquinton’s picture

StatusFileSize
new7.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-new-entity-link-1798540-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Cool - agreed!

Here's a new patch with your suggestions implemented...

damiankloip’s picture

+++ b/core/modules/node/lib/Drupal/node/Entity/NodeType.php
@@ -37,7 +37,8 @@
  *     "add-form" = "node.add",
  *     "edit-form" = "node.type_edit",
...
+ *     "delete-form" = "node.type_delete_confirm",
+ *     "add-entity-form" = "node.type_add"

add-entity-form seems to break the pattern of $action-form.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

At NYCCamp we had a long discussion about the naming, and the weirdness of NodeType having a route for node.add and node.type_add, and what to name them.

If we really care about the naming, we should open a dedicated issue, because I'm pretty sure the existing "add-form" = "node.add", pattern is wrong.

Awesome work @jamesquinton, (hopefully) congrats on your first core patch!

damiankloip’s picture

IMO having the actual node type as add-entity-form is even more confusing. So add-form does is not for the actual entity type of the annotation it's on but for nodes, whereas add-entity-form is for the current entity. Maybe at least add-type-form?

Looks like this was happening before this, but why are node and node type mixing the link templates so much? This just seems wrong.

tstoeckler’s picture

So the reason why "node.add_form" (i.e. node/add/{type}) is on NodeType instead of Node is because it has the node type as a route parameter and not a node. I also think that is fairly weird but I also have no clue how to fix this.

damiankloip’s picture

:/

jamesquinton’s picture

Assigned:jamesquinton» Unassigned

Unasigning myself as I think this is now more of a naming issue now...

I do agree that it seems strange for node.add_form to be on NodeType instead of Node.

Interestingly, custom blocks are kind of similar - but with Custom Block Types, there is "custom_block.type_add".

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 21: drupal-new-entity-link-1798540-20.patch, failed testing.

jamesquinton’s picture

Assigned:Unassigned» jamesquinton
Issue tags:+Needs reroll
jamesquinton’s picture

Assigned:jamesquinton» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new7.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Re-rolled patch.

Xano’s picture

StatusFileSize
new13.25 KB
new14.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,419 pass(es), 624 fail(s), and 990 exception(s).
[ View ]
  1. I changed the link template name from add-entity-form to add, since it's obvious that this is about entities, and the route may not point to form pages. The one for nodes, for instance, points to a page where users can select the type of which they want to add a node.
  2. I added access control to the link. In order to do that I had to change the behavior of EntityAccessControllerInterface a little.
  3. I injected all dependencies into EntityListBuilder.

The last submitted patch, 30: drupal-new-entity-link-1798540-29.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 31: drupal_1798540_31.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new16.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,522 pass(es), 616 fail(s), and 485 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 34: drupal_1798540_34.patch, failed testing.

Xano’s picture

StatusFileSize
new16.55 KB
new28.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,392 pass(es), 150 fail(s), and 120 exception(s).
[ View ]

My apologies for not reading the issue thoroughly enough and explaining why I made some changes. Here's my reasoning

  1. I changed the link template, because not all entity types have a single add form, namely those with bundles. Those have pages you have to select the correct bundle at, before you get to the form. For consistency with the other templates I reverted this change to add-entity-form, though.
  2. I added a few smaller methods to make it easier for child classes to change the human-readable strings for when the table is empty. We have a sane default, but entity type names are too different from one another for this default to be correct in most cases.
  3. I would very much like a review on my EntityAccessControllerInterface change from last patch. It does not break any existing code, but it may not work properly with existing code either. It does allow us to check access to the add-entity-form link template's route for entity types that have bundles, though.

This patch reverts the link template name, and fixes some test failures.

@jamesquinton: I learned that this is your first core issue and you started on it as part of a sprint. I tried to make this patch a starting point for you to finish fixing the test failures and learn something about dependency injection. See the interdiff for how some of the previous failures were fixed. If you are no longer interested or cannot finish this issue, that is fine and I will do it. Otherwise, the honor is yours and I will review your patches.

jamesquinton’s picture

Assigned:Unassigned» jamesquinton

@Xano - thanks for the detailed explanation! Definitely still interested, I'll take a look (hopefully today)...

Xano’s picture

Status:Needs work» Needs review

Cool!

I forgot to trigger the bot. Let's see how much fails.

Status:Needs review» Needs work

The last submitted patch, 36: drupal_1798540_36.patch, failed testing.

jamesquinton’s picture

StatusFileSize
new40.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,054 pass(es), 150 fail(s), and 122 exception(s).
[ View ]

OK - I'm having trouble running tests locally at the moment, but hopefully this should reduce the number of errors...

jamesquinton’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 40: drupal_1798540_40.patch, failed testing.

jamesquinton’s picture

Issue tags:+drupalcampfi
jamesquinton’s picture

Assigned:jamesquinton» Unassigned
Issue tags:-drupalcampfi

I'm starting to think this may be better suited to someone who is more comfortable with dependency injections, as this is still a relatively new concept to me!

I think I will have to bail out of this one, at least until I am a bit more familiar with dependency injections...

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

I think this has gotten a little out of hand with the DI, I don't blame you for being confused @jamesquinton. I'm going to try and get this back on track.

Xano’s picture

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
StatusFileSize
new14.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal_1798540_46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Unassigning, since Tim just left the sprint and is on a plane.

This patch removes the dependency injection.

Xano’s picture

46: drupal_1798540_46.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 46: drupal_1798540_46.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new14.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,922 pass(es).
[ View ]

Re-roll.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -203,6 +208,44 @@ public function render() {
    +    return $this->t('Add one.');

    Did we thought about not using a dot at the end? Did we considered to put @label of $this->enttityType->getLabel() in there?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -203,6 +208,44 @@ public function render() {
    +    if ($this->entityType->hasLinkTemplate('add-entity-form') && \Drupal::entityManager()->getAccessController($this->entityTypeId)->createAccess()) {
    +      return \Drupal::linkGenerator()->generate(

    I can haz injections or at least helper methods?

  3. +++ b/core/modules/node/src/NodeListBuilder.php
    @@ -49,8 +49,7 @@ public function __construct(EntityTypeInterface $entity_type, EntityStorageInter
    +  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {    return new static(
           $entity_type,
           $container->get('entity.manager')->getStorage($entity_type->id()),
           $container->get('date')

    This looks wrong.

Xano’s picture

Did we thought about not using a dot at the end? Did we considered to put @label of $this->enttityType->getLabel() in there?

Yes. See the overrides in the other classes. These are sensible defaults that are grammatically and stylistically correct, as we can't dynamically do that by using entity type names.

I can haz injections or at least helper methods?

It was decided that that's for a follow-up and we focus on just adding the link here.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Yeah, all people know that you are lazy.

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/node/src/NodeListBuilder.php
@@ -49,8 +49,7 @@ public function __construct(EntityTypeInterface $entity_type, EntityStorageInter
-  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
-    return new static(
+  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {    return new static(

This change is just wrong.

The last submitted patch, 49: drupal_1798540_48.patch, failed testing.

Status:Needs work» Needs review

Xano queued 49: drupal_1798540_48.patch for re-testing.

dawehner’s picture

Status:Needs review» Needs work

so.