Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Status: Active » Needs review
FileSize
6.21 KB

Here's a patch.

A few things I'm not sure what to do about:

- entity_ui_bundle_add_page() and entity_ui_get_bundle_add_form() could technically be moved to a .inc file, but the module doesn't have a .admin.inc or .pages.inc, so I wasn't sure about adding one
- unlike node_menu() we only make one menu item for ENTITYPATH/add/BUNDLE, rather than one item per bundle. The main reason for this seems to be that the main 'add' page can be generated with system_admin_menu_block().
- because bundles don't have descriptions, we can't follow the UI pattern of theme_node_add_list(), so I've just done a basic theme_item_list().
- there's no access granularity on the different bundle types. I'm not sure that's in the scope of EntityAPI, though I don't know how a module using this could add that in.

FATALIST’s picture

Thanks, joachim. Works fine for me.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

awesome!
thanks, exactly what i needed:)

ParisLiakos’s picture

fago’s picture

Status: Reviewed & tested by the community » Needs work

While I like the idea we must be careful to not break BC for modules. The patch does remove a menu item upon which current implementations may rely. Modules may rely on anything the existing UI controller does, thus I think it would be best to introduce this as another, alternate UI controller for bundled entities.

joachim’s picture

Status: Needs work » Needs review
FileSize
8.35 KB

That makes sense.

Here's a patch that does the UI controller work in a new subclass.

I'm not sure whether to add the paths below BASE/add as BASE/add/%, or one menu item for each bundle as BASE/add/BUNDLE.

I've made a few more additions to the patch, based on implementations of this UI pattern I've made in custom modules.

PatchRanger’s picture

@joachim Thanks, your patch works for me. Actually it replaced all of my custom stuff, which is doing the same thing: providing consistent add page.

I'm not sure whether to add the paths below BASE/add as BASE/add/%, or one menu item for each bundle as BASE/add/BUNDLE.

My vote is for separate menu item for each bundle: it empowers Admin Menu (and other menu-related modules) to create additional menu items for adding entities of each type, which is great, because avoids custom coding to achieve that.
So (basing on your patch) I've created a new one: see attached, as well as interdiff.

fago’s picture

Status: Needs review » Needs work
+++ b/entity.module
@@ -114,6 +114,54 @@ function entity_object_load($entity_id, $entity_type) {
+ * This shows links to all bundles, as we don't have access permissions at a
+ * bundle granularity.

That might differ for a specific entity type though. We should make entity-access checks on an empty entity having just the bundle set.
If modules use a uid they can default to the current-user in their create() method on the storage controller.

+++ b/includes/entity.ui.inc
@@ -30,7 +30,8 @@ class EntityDefaultUIController {
+    // Set this on the object so classes that extend hook_menu() can use it.
+    $this->id_count = count(explode('/', $this->path));

We should define the protected variable then.

fago’s picture

EntityBundlesUIController

Given our bundles are also entities I think the name might be confusing, maybe better call it "EntityBundleableUIController".

Also it's a bit weird is that we have to use the 'admin ui' keys in hook_entity_info() - but well, we cannot change that anymore so I guess we have to live with that.

dasjo’s picture

i believe fago forgot to mention, that his review was based on #6.

attached is a patch that addresses the comments from #8.

#9 hasn't been implemented!

fago’s picture

Status: Needs review » Needs work

indeed, thanks.

+++ b/entity.module
@@ -114,6 +114,62 @@ function entity_object_load($entity_id, $entity_type) {
+ * This shows links to all bundles, as we don't have access permissions at a
+ * bundle granularity.

Needs to be updated to reflect latest code changes.

+++ b/includes/entity.ui.inc
@@ -493,6 +495,50 @@ class EntityDefaultUIController {
+class EntityBundlesUIController extends EntityDefaultUIController {

We should rename the controller - it's not for entity bundles but for bundlable entities. -> EntityBundleableUIController ? -> See #9.

dasjo’s picture

thanks for the review fago!

i have incorporated your comments into the attached patch.

fago’s picture

Status: Needs review » Postponed

ok, patch looks good. However, one more thing:

This is for content entities, looking closer at it it's a bit weird to have the admin listing at the specified path and no view callback. So let's make this first, then base this upon that.

-> Let's solve #1105618: UI controller for content-entities, then inherit this class from the content UI controller provided there.

dasjo’s picture

PatchRanger’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 1619628-14.entity.entity-add-ui-bundles.patch, failed testing.

fago’s picture

Status: Needs work » Fixed

Thanks, I improved documentation a bit and committed it.

Status: Fixed » Closed (fixed)

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