Problem/Motivation

#2596095: Provide a route provider for add-form of entities was refocused to only add the add-form route.
But any content entity with bundles also needs an add-page, that will allow the user to select a bundle and be sent to the form.
This is generic code and doesn't need to be repeated.

We need to extend the route provider, add a controller method, and add an EntityDescriptionInterface (contrib example) for $bundle_entity->getDescription().

Contrib implementation:
https://github.com/fago/entity/blob/8.x-1.x/src/Routing/CreateHtmlRouteP...
https://github.com/fago/entity/blob/8.x-1.x/src/Controller/EntityCreateC...
https://github.com/fago/entity/blob/8.x-1.x/templates/entity-add-list.ht...

Proposed resolution

Let's add it.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#50 2666792-50-add-page-follow-up.patch1.37 KBkristiaanvandeneynde
#48 2666792-48-add-page-follow-up.patch1.37 KBkristiaanvandeneynde
#47 2666792-47-add-page-follow-up.patch3.19 KBkristiaanvandeneynde
#38 interdiff.txt6.34 KBbojanz
#38 2666792-38-add-page.patch32.21 KBbojanz
#34 interdiff.txt1.38 KBbojanz
#34 2666792-33-add-page.patch32.46 KBbojanz
#32 interdiff.txt5.6 KBbojanz
#32 2666792-32-add-page.patch32.55 KBbojanz
#30 2666792-29-add-page.patch29.92 KBbojanz
#29 interdiff.txt14.7 KBbojanz
#25 interdiff.txt861 bytesdawehner
#25 2666792-25.patch27.72 KBdawehner
#23 interdiff.txt9.83 KBdawehner
#23 2666792-23.patch26.88 KBdawehner
#19 Captura de pantalla 2016-03-07 16.35.55.png23.02 KBedysmp
#19 Captura de pantalla 2016-03-07 16.25.31.png19.45 KBedysmp
#19 2666792-19-add-page-route.patch18.28 KBedysmp
#19 interdiff_16-19.txt782 bytesedysmp
#16 interdiff.txt1.14 KBbojanz
#16 2666792-15-add-page-route.patch18.17 KBbojanz
#14 2666792-14-add-page-route.patch17.03 KBbojanz
#10 interdiff.txt715 bytesdawehner
#10 2666792-10.patch2.79 KBdawehner
#3 2666792-3-add-entity-description-interface.patch2.31 KBbojanz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

bojanz’s picture

Status: Active » Needs review
FileSize
2.31 KB

Added the interface, made it used by NodeType (note that there wasn't a setter before. And for some reason no other setters return $this.)

dawehner’s picture

Mh, so this is sort of a BC break on NodeTypeInterface, but I don't think someone actually implements that again.

bojanz’s picture

Worst case scenario, we skip updating NodeTypeInterface, make this for new code only.

dawehner’s picture

Worst case scenario, we skip updating NodeTypeInterface, make this for new code only.

Yeah no idea. We could also just let NodeType implement the interface, so on runtime you can still check for EntityDescriptionInterface

bojanz’s picture

Title: Add a EntityDescriptionInterface » Provide a route provider for add-page of entities
Issue summary: View changes
Status: Needs review » Needs work

I spoke to berdir. The official position is not to extend existing interfaces. For example: https://www.drupal.org/node/306662#comment-10816832

So let's leave the core conversion battle for a different issue. We can refocus this issue on adding the interface and using it for the add-page route, since that part was removed from #2596095: Provide a route provider for add-form of entities

bojanz’s picture

Issue summary: View changes
dawehner’s picture

Title: Provide a route provider for add-page of entities » Add a EntityDescriptionInterface

Revert the title change ...

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
715 bytes

There we go.

Status: Needs review » Needs work

The last submitted patch, 10: 2666792-10.patch, failed testing.

dawehner’s picture

Title: Add a EntityDescriptionInterface » Provide a route provider for add-page of entities

Take it bojanz!

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bojanz’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
17.03 KB

Status: Needs review » Needs work

The last submitted patch, 14: 2666792-14-add-page-route.patch, failed testing.

bojanz’s picture

Status: Needs work » Needs review
FileSize
18.17 KB
1.14 KB

Added missing stable template override.

heddn’s picture

The twig template here seems identical to the one used in node module. But the interesting thing is that the node-add-list.html.twig is then overridden in Seven theme so it looks pretty. But this one looks really ugly. Could we use the Seven style twig template instead of the node module's? Screenshots and updated patch to follow.

heddn’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
    @@ -53,6 +59,13 @@ class EntityController implements ContainerInjectionInterface {
        *
    
    @@ -79,12 +98,67 @@ public static function create(ContainerInterface $container) {
    +    $bundle_type = $entity_type->getBundleEntityType();
    

    This returns a ConfigEntityType in my local setup. We need EntityDescriptionInterface to loadBundleDescriptions() down in line 149.

  2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
    @@ -79,12 +98,67 @@ public static function create(ContainerInterface $container) {
    +    $bundles = $this->loadBundleDescriptions($bundles, $bundle_type);
    

    This needs to be a EntityDescriptionInterface, but it isn't.

edysmp’s picture

edysmp’s picture

Issue summary: View changes

Before:
Before
After:
After

Note that in the after, the description still doesn't appear.

bojanz’s picture

Thanks, edysmp!

The description won't appear until bundle config entity types (such as NodeType) implement the EntityDescriptionInterface. That's normal.

dawehner’s picture

Working on some integration test coverage now.

dawehner’s picture

I moved this special template from stable to seven. I'm not convinced of nesting div elements into a a element, but yeah this is what is done for node as well.

Status: Needs review » Needs work

The last submitted patch, 23: 2666792-23.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.72 KB
861 bytes

:(

Status: Needs review » Needs work

The last submitted patch, 25: 2666792-25.patch, failed testing.

heddn’s picture

re #23: should we still leave a basic template in the system module? As to copy node/seven more closely?

heddn’s picture

Ignore me. I see we do have a copy in the system folder.

bojanz’s picture

Status: Needs work » Needs review
FileSize
14.7 KB

I have once again added an exact copy of entity-add-list.html.twig to the Stable theme, since there's a test that checks whether each module template has a matching Stable template.

Also reworked the logic that distinguishes between entity and non-entity bundles.

We now have green entity bundle tests, but we also need to test non-entity bundles.

bojanz’s picture

dawehner’s picture

The non bundle entity type fallback is looking pretty good.

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
@@ -79,12 +99,76 @@ public static function create(ContainerInterface $container) {
+      $build['#cache']['tags'] = $entity_type->getListCacheTags();

It is still weird to do that, to be honest. For example for 'user' this would mean that this page is uncached, everytime a user joins/updates itself. This doesn't really make sense for me at least.

bojanz’s picture

You're right, removed it.

Also fixed a notice, and added the test coverage for non-entity bundles.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/system/tests/modules/entity_test/src/Routing/EntityTestRoutes.php
@@ -25,11 +25,6 @@ public function routes() {
-      $routes["entity.$entity_type_id.add_form"] = new Route(
-        "$entity_type_id/add",
-        array('_entity_form' => "$entity_type_id.default"),
-        array('_permission' => 'administer entity_test content')
-      );

Nice, that this was actually not used by something.

bojanz’s picture

Forgot to sync the seven template. Now done.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Generally looks good, only the last point is substantial at all:

  1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
    @@ -209,4 +292,32 @@ protected function doGetEntity(RouteMatchInterface $route_match, EntityInterface
    +   *   The id of the bundle entity type.
    

    ID.

  2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
    @@ -209,4 +292,32 @@ protected function doGetEntity(RouteMatchInterface $route_match, EntityInterface
    +    if (!$bundle_entity_type->isSubclassOf('\Drupal\Core\Entity\EntityDescriptionInterface')) {
    

    Why isSubClassOf and not instanceof?

  3. +++ b/core/modules/system/system.module
    @@ -317,6 +325,36 @@ function system_theme_suggestions_field(array $variables) {
    +function template_preprocess_entity_add_list(&$variables) {
    +  if (!empty($variables['bundle_entity_type']) && empty($variables['bundles'])) {
    +    $bundle_entity_type = \Drupal::entityTypeManager()->getDefinition($variables['bundle_entity_type']);
    +    $label = $bundle_entity_type->getLowercaseLabel();
    +    $link_text = t('Add a new @entity_type.', ['@entity_type' => $label]);
    +    $link_route_name = 'entity.' . $bundle_entity_type->id() . '.add_form';
    +    $variables['add_bundle_message'] = t('There is no @entity_type yet. @add_link', [
    +      '@entity_type' => $label,
    +      '@add_link' => Link::createFromRoute($link_text, $link_route_name)->toString(),
    +    ]);
    +  }
    +
    +  foreach ($variables['bundles'] as $bundle_name => $bundle_info) {
    +    $variables['bundles'][$bundle_name]['description'] = [
    +      '#markup' => $bundle_info['description'],
    +    ];
    +  }
    +}
    

    Why is this done in preprocess and not in EntityController::addPage()?

bojanz’s picture

Assigned: Unassigned » bojanz

Why isSubClassOf and not instanceof?

We are checking whether the entity class implements the interface, not the entity type.
isSubclassOf() is still the main way of doing that, no?

Why is this done in preprocess and not in EntityController::addPage()?

Mostly because template_preprocess_node_add_list() does something similar, and the approach carried over.

I would keep the #markup part in the preprocess.
It allows the following part of the docblock to stay true (and simple):

bundles: An array of bundles with the label, description, add_link keys.

I'll move the add_bundle_message logic to EntityController::addPage().

Berdir’s picture

We're considering to deprecate isSubclassOf, it has caused confusions like this multiple times, see #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements().

bojanz’s picture

Status: Needs work » Needs review
FileSize
32.21 KB
6.34 KB

Replaced "id" with "ID", moved the add_bundle_message logic to the controller, tweaked comments.

I can replace isSubclassOf() if you wish, but to me it's just an uglier version of is_subclass_of($entity_type->getClass(), '')...

Berdir’s picture

I guess you meant to say a nicer version of? :)

Yes it is, but the problem with it is that it's quite confusing, I've seen that feedback/question from reviewers in quite a few issues. And if we're really going go deprecate it, then it's one call less to worry about.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah I totally believe arguing about that is out of scope of this issue.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK I hadn't seen #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() but clearly I got confused the same as other people have, so following that issue now. Agreed it's out of scope here and sorry for the distraction.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

catch’s picture

Issue tags: +8.1.0 release notes
kristiaanvandeneynde’s picture

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestWithBundle.php
@@ -0,0 +1,77 @@
+ *   links = {
+ *     "canonical" = "/entity_test_with_bundle/{entity_test_with_bundle}",
+ *     "add-page" = "/entity_test_with_bundle/add",
+ *     "add-form" = "/entity_test_with_bundle/add/{entity_test_bundle}",
+ *     "edit-form" = "/entity_test_with_bundle/manage/{entity_test_with_bundle}/edit",
+ *     "delete-form" = "/entity_test_with_bundle/delete/entity_test_with_bundle/{entity_test_with_bundle}",
+ *   },

The edit-form and delete-form paths seem a bit weird? Normally you find "manage" in the path for config entities and the delete-form path just looks weird altogether :)

Also, semi-on-topic: I can't find add-page (nor add-form) in http://www.iana.org/assignments/link-relations/link-relations.xhtml, only create-form. Shouldn't we prefix that with "drupal:" like we do in other places in core? I am having a hard time understanding why we sometimes prefix them and sometimes we don't.

Other than that I'm really excited about this patch. It also finally adds a test for bundle entities! That one had been blocking #2645202: Re-saving a new bundle entity throws an exception for a while. I guess I'll have to reroll that one's tests because I assume this one will go in first.

Edit: It got committed while I was reviewing :o

bojanz’s picture

@kristiaanvandeneynde
We have no rule that '/manage/' should be present in the urls, especially for test entities, right?
The delete-form url definitely looks wrong (though it will work).

kristiaanvandeneynde’s picture

@bojanz I was pointing out that it's in there, not that it should be in there :)

Those two should probably look like:

"edit-form" = "/entity_test_with_bundle/{entity_test_with_bundle}/edit",
"delete-form" = "/entity_test_with_bundle/{entity_test_with_bundle}/delete",
bojanz’s picture

@kristiaanvandeneynde
Ah, you're right. We should fix that in a followup.

kristiaanvandeneynde’s picture

Status: Fixed » Needs review
FileSize
3.19 KB

Fixed a few trailing commas while I was at it. I recall reading somewhere YAML doesn't like that?

kristiaanvandeneynde’s picture

bojanz just told me Drupal fixed the issue with trailing commas, so new patch.

dawehner’s picture

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestWithBundle.php
@@ -44,8 +44,8 @@
+ *     "delete-form" = "/entity_test_with_bundle/{entity_test_with_bundle}/delete"

Having the comma here would still be great

kristiaanvandeneynde’s picture

Ugh, was trying to be too fast.

The last submitted patch, 48: 2666792-48-add-page-follow-up.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

No worries

catch’s picture

Status: Reviewed & tested by the community » Fixed

Follow-up makes sense, but please post it to a new issue linked from here.

catch’s picture

Status: Fixed » Reviewed & tested by the community

Meh it's just removing two words. I'll commit if it comes back green from the bot, if not new issue please though.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

  • catch committed 076ebd3 on 8.2.x
    Issue #2666792 by bojanz, dawehner, edysmp, heddn: Provide a route...

  • catch committed 3a7869c on 8.2.x
    Issue #2666792 by bojanz, dawehner, kristiaanvandeneynde, edysmp, heddn...

Status: Fixed » Closed (fixed)

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

xjm’s picture

Title: Provide a route provider for add-page of entities » [Needs change record] Provide a route provider for add-page of entities
Status: Closed (fixed) » Needs work
Issue tags: +Needs change record

This issue is missing a change record. (We should add change records for noteworthy API additions.)

xjm’s picture

We can probably create a combined CR for this and #2596095: Provide a route provider for add-form of entities.

dawehner’s picture

I'll write one for all of the 4 entity related ones: https://www.drupal.org/node/2709511

dawehner’s picture

Title: [Needs change record] Provide a route provider for add-page of entities » Provide a route provider for add-page of entities
Status: Needs work » Fixed

Added stuff to the change record. Feel free to improve it.

Status: Fixed » Closed (fixed)

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