The edit links in the admin UI are to /manage/ID.

'edit' is more the norm, surely?

CommentFileSizeAuthor
#6 entity.bundle-ui.6.patch5.13 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Status: Active » Closed (works as designed)

hm, no actually I think it's 'manage', e.g. 'admin/structure/types/manage/article/edit'.

edit is supposed to be after the id, but it is the default task so it usually doesn't appear.

joachim’s picture

Category: bug » feature
Status: Closed (works as designed) » Active

Ok, I see where this is going wrong.

I'm trying to use the admin UI to edit entities, not bundles -- I want a UI to edit party entities. These should be /edit same as nodes or profiles. If you wanted to replicate the admin/content/node for an entity, you'd want /edit.

There probably needs to be a way to specify what the path is for editing.

fago’s picture

I see - I guess we should $entity_uri / edit anyway if we have one. I'm not sure though whether we should handle the 'admin/content/node' use case with the same controller though - as the UI is probably a bit different and one probably needs a different a loading strategy for entities (don't load all of them!) + a pager.

joachim’s picture

I should maybe push what I have so far for my party module to git so we can get more of an idea of implementation cases for this UI system.

sun’s picture

Category: feature » task
Priority: Normal » Major

If the current UI code is about entity bundles (which I totally expected), then why does the code load actual entities?

  public function overviewTable($conditions = array()) {
    $entities = entity_load($this->entityType);
sun’s picture

Status: Active » Needs review
FileSize
5.13 KB

Attached patch fixes the bundle UI.

joachim’s picture

Well it's about both, that's part of the problem...

fago’s picture

Status: Needs review » Needs work

>If the current UI code is about entity bundles (which I totally expected), then why does the code load actual entities?

Not really. It's about entities, but entities that might serve as bundles of other entities - or are just configuration by them self. The UI doesn't require bundles or fields at all - e.g. the wsclient module uses it to let admin manages "web service descriptions", thus there are no fields or bundles involved at all.

That said, I'd say the admin UI is intended to be used for configuration items, not site content. Usually, that items should be exportable, but I don't see any technical cause to require that - however perhaps it would make sense to do that just for making its intended usage more obvious.

>I see - I guess we should $entity_uri / edit anyway if we have one.
Thinking about that, I see no point in having an entity-view somewhere else. If the view makes sense for the entity type, I guess it should live at path/manage/name.

sun’s picture

Title: 'manage' in path rather than 'edit'? » Entity bundles as entities is confusing

I think I get the basic idea behind the current code. However, this is stretching the (core) entity API a bit too far, IMHO. The complexity and amount of conditional code of those form callbacks basically prove that. The operations for entity bundles are entirely different than for entities themselves. Likewise, the concepts and logic applied to bundles and entities elsewhere in (core and contrib) code and modules are different.

Has there been any formal discussion on this architectural idea?

I mean, fundamentally, the idea is probably not wrong. However, the consequence of this architecture is that those fake/bundle entities will be exposed as real entities elsewhere in the system. E.g., modules that are generically acting on entities like http://drupal.org/project/relation have no way to know whether they are dealing with such a fake/bundle entity that doesn't make sense to expose to users.

The part that definitely makes sense is that Entity module tries to take over bundle management for the integrating module. The same can be achieved by exposing dedicated API functions to do that.

joachim’s picture

> Has there been any formal discussion on this architectural idea?

I'm not sure there has, and I agree there ought to be :)

I might add another small note -- having to use hook_entity_info_alter() to register entities, in order to prevent recursion, (see profile2 for an example) is a bit of a DX WTF.

fago’s picture

> Has there been any formal discussion on this architectural idea?
Does drupal has "formal" discussions at all? ;P But anyway, note that also core has an bundle implemented as entity: vocabularies. Also, don't forget this is contrib where we have the freedom of choice to test new concepts.

In the end it boils down to what we think an "entity" is - I'd see an entity as an arbitrary storage object, which doesn't imply its fieldable or whatever. For an entity in core, it's just clear there is a way to load it. Anything else, labels, uris, fields,.. are optional. The entity CRUD API, extends that R to CRUD - however I do think also CUD should remain optional.

Thus, having an API for CRUD, I see no cause why one should re-invent yet another CRUD API for bundle objects when we have already one. Thus said, I really think the foundational CRUD API has to be common, while anything on top of it might differ to handle the necessary differences, e.g. between content and configuration.

>However, the consequence of this architecture is that those fake/bundle entities will be exposed as real entities elsewhere in the system.

Yes, but again it is already that way in core - e.g. there is the 'file' entity which makes not much sense to be exposed to users in many cases either. So I agree that there need to be ways for modules to determine which entity types need to be exposed, but that also depends on the use. As of now, you could only handle entities being fieldable, or you could handle all entities being not 'exportable' with the entity API what would include the file entity type too.

sun’s picture

I'm warming up to the idea of declaring entity bundles as entities in the meantime. However, it's pretty much clear that the architecture needs work; specifially we need to:

  1. Explicitly state which entity is a bundle entity and which is not.
  2. Make pointers to bundle entities declarative (instead of inherited).

Details:

    • Mixing configuration entities into user content/data entities, with regard to other use-cases, is a troublesome idea.
    • Even though bundle entities are technically protected by access permissions, it does not make any sense to evaluate them for user data use-cases in the first place.
    • To start with a simple consideration, an entity info property like
        // One of 'content|data|user', 'configuration|bundle'.
        'type' => 'configuration',
      

      would already be a giant help.

    • Of course, the very next best question is whether all possibly existing entities can be categorized in those two (or potentially other, custom) types, and in turn, whether the value should be an array instead of a single string. Thus, if we'd want to be on the safe side and account for yet unknown use-cases of entities, then we'd do:
        // One or more of 'content|data|user', 'configuration'.
        'type' => array('configuration', 'strange'),
      

      Makes sense?

    • Bundle entities are currently registered as separate entities, using a custom entity info property:
        'bundle of' => 'some_entity_type',
      
    • However, entity bundles resp. bundle entities are highly specific and tailored to a certain entity type, as they carry essential properties required for the entity type.
    • While there might be a special use-case (that I can't think of) of replacing an entity's bundle entity with a different bundle entity to facilitate site-specific customization or something similar, I really do not get the point of inheriting bundle entities to an entity type.
    • In other words, I don't see how the following is going to work and can't think of any remotely valid use-case for doing it:
      $types['foo'] = array(
        ...
      );
      $types['foo_type'] = array(
        'bundle of' => 'foo',
      );
      $types['bar'] = array(
        'bundle of' => 'foo',
      );
      $types['bar_type'] = array(
        'bundle of' => 'bar',
      );
      
      1. First of all, the bundle entity 'foo_type' is specific to 'foo'. That alone would have to enforce that 'bar' is identical to 'foo_type'.
      2. Even when considering the edge-case that 'bar' is a type of 'foo' and identical to 'foo_type', then how does the module exposing the entity type 'foo' know of 'bar' in the first place (and account for multiple bundle entity types)?
      3. A technically possible variant 'bar_type' of 'bar' that is itself a variant of 'foo_type', which is a variant of 'foo' is plain simply nuts.
    • With "declarative", I mean to specify a 1:1 relationship between entity type and bundle entity type, but don't allow for anything else:
      $types['foo'] = array(
        'type' => 'content',
        'bundle entity' => 'foo_type',
      );
      $types['foo_type'] = array(
        'type' => 'configuration',
      );
      

      At least that would make a lot more sense to me.

fago’s picture

ad 1)
Hm, better not introduce a 'type' of an 'entity type', which might have types itself (node). :D We could go with 'tags' though.
Anyway, coming up with the right categorization is for sure not simple. I'd even say categorizing in content <-> configuration is not possible, as it depends on how the stuff is used.

E.g., are terms configuration or content? If I use a taxonomy to provide a list of values users can pick of, it's configuration. If I use a free-tagging widget though, it's content. Then, there are vocabularies - which would be clearly configuration to me. Still there are modules creating vocabularies on the fly (e.g. og_vocab). I'd argue it's still configuration though.

So I'm unsure whether configuration <-> content is the right distinction we need to make.

Then there is configuration, which isn't a bundle of another entity at all - so whether something is an bundle is rather un-important for that question I think.

>Even though bundle entities are technically protected by access permissions, it does not make any sense to evaluate them for user data use-cases in the first place.

So you'd not allow vocabularies to be used either? What about files? Then there might be entities, that are technically content, but are just behind-the-scences objects a module makes use of. I guess they should not be user-facing either?

ad 2)
Enforcing a 1:1 relationship makes much sense to me. I did not consider having anything else, the 'bundle of' property just ended up to be in the bundle-entity-info as it makes checking whether the current entity type is a bundle of which entity easy. 'bundle of' just makes sure the bundle-field-attachers get invoked, as core doesn't assume the bundles are entities. So you still have to specify the usual bundle information on the fieldable entity type.

Having 'bundle entity' => 'foo_type', makes more sense, but requires parsing the whole entity info to detect whether the given entity is a bundle. Do you think that's worth it? Maybe we should just improve the docs.

naught101’s picture

subscribe

sun’s picture

Yes, I think it's really worth to change bundle of into a declarative bundle entity, especially given the weird inheritance issues of bundle of outlined in #12.

The goal should be to not require code like the following:

/**
 * Implements hook_entity_info_alter().
 *
 * Use this hook to specify profile bundles to avoid a recursion, as loading
 * the profile types needs the entity info too.
 */
function profile2_entity_info_alter(&$entity_info) {
  foreach (profile2_get_types() as $type => $info) {
    $entity_info['profile2']['bundles'][$type] = array(
      'label' => $info->label,
      'admin' => array(
        'path' => 'admin/structure/profiles/manage/%profile2_type',
        'real path' => 'admin/structure/profiles/manage/' . $type,
        'bundle argument' => 4,
        'access arguments' => array('administer profiles'),
      ),
    );
  }
}
fago’s picture

>The goal should be to not require code like the following:

This won't be possible, as the field API requires us to make those definitions in hook_entity_info() - and the only way to do that with bundles-as-entities is doing it in alter. This is, as retrieving bundle objects, means loading entities again, which requires entity-info too. Thus this problem is there, regardless of how we specify the bundle-relation for the entity-CRUD API.

However, if we'd could assume any bundle-objects are entities (-> d8), we could completely discard this duplicated information, but let the field-system retrieve it from entity_load() / the bundle's entity-info itself. That would be the logical consequence of what you are suggesting - specify the 'bundle' entity in hook_entity_info() and read everything.

Anyway, the 'bundle entity' key as in

$types['foo'] = array(
  'type' => 'content',
  'bundle entity' => 'foo_type',
);

makes much sense to me too. In particular as I think it might work to have the same bundle entity 'foo_type' for multiple different 'entity_types'. At least conceptually, it makes sense.

pjcdawkins’s picture

Component: Entity CRUD API - UI » Core integration

subscribe (also found 'bundle of' confusing)

(edit: oops it turns out I found this issue twice, for different reasons: do excuse this post)

scroogie’s picture

Isn't the original issue of exposing entity view and edit capabilities on $entity/edit $entity/view etc. nearly solved through #1196456: provide a harmonized entity_form() function? It would just need an additional controller, or not?

bojanz’s picture

Category: task » support
Status: Needs work » Fixed

We've accepted entities as bundles to other entities a long time ago. Time to close this.

Status: Fixed » Closed (fixed)

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