Comments

joachim’s picture

Comment #1

joachim commented
Status: Active » Needs review
StatusFileSize
new 1619628.entity.entity-add-ui-bundles.patch 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

Comment #2

FATALIST commented

Thanks, joachim. Works fine for me.

ParisLiakos’s picture

Comment #3

ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

awesome!
thanks, exactly what i needed:)

ParisLiakos’s picture

Comment #4

ParisLiakos commented
fago’s picture

Comment #5

fago
Primary language German
Location Vienna
commented
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

Comment #6

joachim commented
Status: Needs work » Needs review
StatusFileSize
new 1619628-6.entity.entity-add-ui-bundles.patch 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

Comment #7

PatchRanger commented
StatusFileSize
new interdiff.txt 1.98 KB
new entity-1619628-7-ui_for_bundles_add_page.diff 8.55 KB

@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

Comment #8

fago
Primary language German
Location Vienna
commented
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

Comment #9

fago
Primary language German
Location Vienna
commented

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

Comment #10

dasjo
he/him
Primary language German
Location Zurich
commented
Status: Needs work » Needs review
StatusFileSize
new 1619628-10.entity.entity-add-ui-bundles-interdiff.txt 1.42 KB
new 1619628-10.entity.entity-add-ui-bundles.patch 8.95 KB

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

Comment #11

fago
Primary language German
Location Vienna
commented
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

Comment #12

dasjo
he/him
Primary language German
Location Zurich
commented
Status: Needs work » Needs review
StatusFileSize
new 1619628-12.entity.entity-add-ui-bundles-interdiff.txt 993 bytes
new 1619628-12.entity.entity-add-ui-bundles.patch 8.85 KB

thanks for the review fago!

i have incorporated your comments into the attached patch.

fago’s picture

Comment #13

fago
Primary language German
Location Vienna
commented
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

Comment #14

dasjo
he/him
Primary language German
Location Zurich
commented
PatchRanger’s picture

Comment #15

PatchRanger commented
Status: Postponed » Needs review

Comment #16

Status: Needs review » Needs work

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

fago’s picture

Comment #17

fago
Primary language German
Location Vienna
commented
Status: Needs work » Fixed

Thanks, I improved documentation a bit and committed it.

Comment #18

Status: Fixed » Closed (fixed)

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