Posted by Dave Reid on November 4, 2011 at 3:58pm
31 followers
Jump to:
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | entity system |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | active |
| Issue tags: | Configurables, Contributed project blocker |
Issue Summary
For modules that want to operate on publicly-visible entities, we need a reliable method to determine which entity types are not. See #1330546: Pathauto should work with all entity types and #1286122: Incompatibility with Commerce for a little more history. We have some problems with core and contrib entities:
- The comment_uri callback creates links to 'comment/1' URLs which are handled by comment_permalink() which essentially redirects the user to the node page with that comment. I propose that the 'uri_callback' should be removed from comment_entity_info() as comments cannot be viewed individually.
- Commerce entities implement URI callbacks. But for products these end up by admin pages by default and not actually public-visible URIs.
All of our entities except payment transactions have fields and paths, but none of them will be directly displayed to end users or accessible to search bots.
I would propose commerce removes the support for uri_callback unless these entities are actually publicly visible.
Comments
#1
Yay config_test violates this with its test entity now too.
#2
I get the fundamental issue you're after here, but I strongly disagree with two points:
1) The comment is shown on the returned URI, and it's the comment's primary URI to view it. So that's exactly correct. The redirect is suboptimal, but that doesn't change the situation.
2) The required access level to an entity doesn't change the fact that the [e.g., product] entity can be viewed on the given URI. As long as that is the case, that's the URI of the entity and it should be returned. "Public-facing" is irrelevant; the caller needs to ensure view access to the entity anyway.
What I can get behind is the case of config entities, which do not have a "page view" of their own (...yet?).
Right now, the only "view" we're only ever showing for config entities is the "edit form/view", which thus can be interpreted as the entity's primary URI, although I admit it slightly muddies the waters.
However, if we don't do that, then we'd need to introduce a new entity metadata key along the lines of
'edit uri callback'... — which inherently leads to the need for a'delete uri callback', etc.pp... mess guaranteed.#3
Let's step back slightly and think about what we really do need:
- I agree with sun that these entities still need a URI callback -- they do have URIs, need to be accessed, and the uri callback should be the way to provide them (even if, in the case of comments, it's a redirect), because it's used for other purposes in core.
- I agree with Dave Reid that there are reasons (such as Pathauto) that we need a way to figure out whether a given entity type has its own real public-facing URL vs. an admin URL vs. a redirected URL.
So... here are some thoughts on how we could potentially differentiate between these cases:
a) Add a component to hook_entity_info() or the entity class or something called something like "uri_is_public" [think of a better name?]. This would indicate that (1) the entity has its own URI (not a redirect) and (2) it is often/usually/normally for public viewing rather than admin viewing. Alternatively, we could potentially have one component called 'admin' indicating it's an adminstrative/configuration type of entity, and another one called 'uri is redirected'?
b) Add an element to the uri callback return value called "public" [again, better name?] indicating the same as (a), or again maybe two components for these two different concepts.
Thoughts? If Pathauto [one of the 3 most popular contrib modules] wants to be able to apply itself to all entities, and we're using a *lot* of entities in core for configuration/administrative things that definitely don't want auto-URLs, then we do need a way to distinguish these things.
See also #229568: [META] Pathauto in Core
#4
My proposal would be to move
EntityInterface::uri()toContentEntityInterface::uri()and then create aConfigEntityInterface::adminUri()for the admin entities. I thought about all use-cases currently in core and that seems to map to the expectation we have in core anyway. And if we do find the edge-case where e.g. a ConfigEntity needs a uri() *and* an adminUri() there's nothing that prevents a specific FooConfigEntity from implementing ContentEntityInterface additionally. (I didn't actually check whether PHP is smart enough to allow that, though.) I must admit that I did not think about this excessively and I might have missed something important.For the 'redirect' thing. The URI returned from EntityInterface::uri() already has an options array, so we could easily add a
'redirect' => TRUEor something similar, right?Note: Regarding the name of a possible ConfigEntityInterface::adminUri() our current standards would actually dictate adminURI(), but then we should also have EntityInterface::URI(), so...
#5
Minor note: the 'options' array is sent as $options to the url() function, and that is what it's meant for, so I think it would make more sense to have any additional items in the URI callback return value be their own array elements.
#6
Right, that makes sense. I didn't think about that.
#7
OK, wait a moment... So you're saying that all entities that are meant to be config should/must implement ConfigEntityInterface and content ones should/must implement ContentEntityInterface? (Please excuse my ignorance of this -- just trying to move this issue forward, and I haven't been following all the developments in D8.). So that seems like it could be a way for Pathauto to distinguish them. Pathauto would want to make a list of all entity types that are ContentEntityInterface. Is there a way to make such a list?
Assuming there is, all we would have left to do is the redirect piece, and I don't think we'd even need to worry about moving/renaming the URI methods as suggested in #4. All we would need to do is:
a) document on hook_entity_info and/or interfaces that in your URI callback, in addition to 'path' and 'options', you should set 'redirect' => TRUE if your URI is a redirect.
b) set this in the Comment entity's function/method.
Hm. I see there is no more hook_entity_info() and everything is documented on EntityManager now and it's set up in plugin annotation. Well anyway. Something like this... but I'm not sure my assumptions are correct:
- Do all content entities implement ContentEntityInterface?
- Is there a way to list them? [I guess there would be: EntityManager has a way to get all the entity definitions, and you could go through them and check if their classes implement ContentEntityInterface].
#8
ConfigEntityInterface is for CMI entities. Being a CMI entity and being an admin entity seems to be two different concepts.
Let me add two new use cases:
Commerce Products
admin/commerce/product/<product_id>Commerce Orders
Those are both administrative and content entities, they have three (!) URIs:
admin/commerce/order/<order_id>checkout/<order_id>user/<user_id>/order/<order_id>One might argue that the last two URIs are aliases to each other. It is only partially true: both are attached to different sections of the site, and as a consequence can belong to different menus / theme / etc.
#9
I think we should just start accepting that entities have several URIs and design an API around that.
This has been acknowledged multiple times by the W3C. For example, On Linking Alternative Representations To Enable Discovery And Publishing (from 2006) concludes:
#10
fago, scor, and I just discussed this option. The JSON-LD needs a URI to identify every entity, including ConfigEntities and others that don't have corresponding HTML representations, as explained in #1803586: Give all entities their own URI.
Adding a flag or enumeration to the array that uri() returns seems viable. Then calling code could check that flag before using the URI in an HTML link.
#11
Just a note that assuming #1751210: Convert URL alias form element into a field and field widget is done, path aliases will become a field, meaning the site builder will have control of putting that field on entities or not putting it on, so the whole question of "knowing which entities does pathauto apply to" becomes moot (it would apply to this new path alias field).
#12
Super-closely related:
#1783964: Allow entity types to provide menu items
I fear we need a meta issue to consolidate all of those discussions and change proposals. :-/
#14
The problem with just adding a flag to entity_uri() itself, is several of the modules need to know without having any kind of entity object, on a generic basis which entity types will have public/final URIs.
#15
FWIW: admin_menu for D8 automatically expands all config entities that have a URI in the Administration menu now:
#1899820: Automatically create menu expand mappings for all configuration entity types
http://drupalcode.org/project/admin_menu.git/blob/refs/heads/8.x-3.x:/ad...
Works excellently, but some config entities do not provide any URI information at all right now, so work on this also triggered:
#1899816: Various config entity types are missing URI callbacks
This possibility is critical for me, so I think the entire focus of this issue is really on the idea of public entity URIs vs. administrative entity URIs.
In my research, I learned that there is literally no way to identify "Configurables" right now - a config_prefix property in the entity info is only the starting point, but in reality, that's merely a metadata property for ConfigStorageController to work. If you'd replace that storage controller with a different (e.g., because you want to keep everything in the DB), then it wouldn't be 100% reliable anymore.
Likewise, render controller and list controller definitions, and all other entity info properties we currently have do not imply any further meaning than their concrete meaning, so they cannot be used as a measure to differentiate entity types either.
#16
What about \Drupal\Core\Config\Entity\ConfigEntityInterface?
#17
That helps distinguish between content and config entities, but still doesn't help entity types in distinguishing between can have public URIs vs not.
#18
#1803586: Give all entities their own URI has made this a major task as it completely blocks several contrib modules from any kind of public vs private entity URI logic.
#19
#20
We can probably add a "routable" flag to the annotation for an entity type. Then modules can make decisions based on that value.
I still want to eliminate uri callbacks entirely now that we have a url() method, since there's no value to splitting it off into another callback layer.