Posted by yched on July 20, 2009 at 11:51pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Needs Documentation, Needs tests |
Issue Summary
Forked from #391330-41: File Field for Core:
The image_link_content formatter raises an interesting issue: we currently have no generic, reliable way to generate the url of an 'object' (node, user, comment, taxo term, contrib fieldable entity...). The patch assumes "{$object_type}/{$object_id}", which happens to work for nodes, users and comments, but is obviously not future proof.
Not sure how best to address that : a (pseudo-)token string in hook_fieldable_info() ? a new (fieldable type) hook that builds a url ?
Marking this 'critical' (as in: we need to solve this before we ship)
Comments
#1
oh totally, i hope this doesn't end up totally tied to the fields API because we really need similar functionality for the files table. over on #353458: hook_file_references() was not designed for a highly flexible field storage i'm trying to kill hook_file_references() and move it to a table so that user, nodes, comments, what ever can hold a reference to a file and this would be a great way to build a link to the them for administrative purposes ala #322287: Add file.module to provide UI for managing files.
#2
Well, currently hook_fieldable_info() is the only cross-entity hook we have in core...
If/when #460320: Standarized, pluggable entity loading (nodes, users, taxonomy, files, comments) lands, then hook_fieldable_info() becomes hook_entity_info() (with *optional* properties for the 'fieldable' behavior) and it will be a 'not fieldable-only' home for this.
All the entity types you mention (user, nodes, comments) are or will soon be fieldable, BTW. Once File field lands and replaces upload.module, what's the other way for an object to reference a file than through a File Field ?
#3
Subscribing as #16161: Move Read More link to end of node content would benefit from this too.
I'm new to the whole Fields API story, but as I understand it, one of the main benefits of fields is their flexibility. In that light I think a hook or callback solution (with default fallback {type}/{id}) is preferred over the pseudo token string.
#4
Renaming to match the now officially official term of 'entity'.
#5
Suggestion:
Let's add a "view path" to entity declarations in hook_entity_info. This could then contain an arbitrary URL, with a wildcard for the ID (or, even better, for an arbitrary field of the entity).
So e.g in node_entity_info() you'd had
<?php$return['node']['view path'] = 'node/%nid';
?>
and to create the link to an entity, you'd match the wildcard and replace it with the identically named property of the actual entity object.
This would provide a very flexible solution also for not-so-standard entity types, and could also be altered by contrib through hook_entity_info_alter().
#6
The attached patch adds a function to create the URL to an arbitrary entity, following the suggestion I outlined above.
It's working well. I think this solution should be flexible enough.
Note that this is not yet integrated in field API. I didn't find a spot so I'm leaving this to the field api guys.
#7
Looks good to me, thanks for tackling this Frando :-).
+++ includes/common.inc@@ -5165,6 +5165,37 @@ function entity_get_controller($entity_type) {
+ * @param $entity
+ * An actual entity instance for which the path shall be created.
+ *
+ * @return
I think we don't add empty lines before @return
'an actual entity instance' ? couldn't this simply be 'the entity' ?
+++ includes/common.inc@@ -5165,6 +5165,37 @@ function entity_get_controller($entity_type) {
+ preg_match('!%([^/]+)(/|$)!', $info['view path'], $matches);
+ // Get the property of the entity that has the name of the wildcard.
+ $property = $entity->{$matches[1]};
We'd possibly need to support several replacements ?
+++ modules/system/system.api.php@@ -61,6 +61,10 @@
* - cacheable: A boolean indicating whether Field API should cache
* loaded fields for each object, reducing the cost of
* field_attach_load().
+ * - view path: A pattern for the menu router path to view entities. The
+ * pattern can contain a wildcard that will be replaced by the entity's
+ * property of the same name (if the view path pattern is 'node/%nid',
+ * %nid will be replaced by the entity's nid property).
indentation is off
+ We'd also need comment/%cid for comments
First use cases will come with Filefield / imagefield, we can commit something without waiting for them.
This review is powered by Dreditor.
#8
re-roll to chase head, but didn't include all of yched's comments from #7, thus leaving as 'needs work'.
In particular, still need to address
and
.
#9
re-roll to incorporate yched's:
comment/%cidfor comments#10
The last submitted patch failed testing.
#11
Re-rolled with better tools, and fixed an error I had made.
#12
Looks proper to me .. .since file module is in core now, do we have a use case yet? regardless, this is ready.
#13
It's actually imagefield that will have use cases, so this can go in as is IMO.
#14
Very elegant.
+ $pieces = explode('/', $path);+ // Replace wildcards in the view path.
+ foreach ($pieces as $piece) {
+ // Match the wildcard
+ if (preg_match('!(%[^/]+)(/|$)!', $piece, $matches)) {
The regexp looks too complex. $pieces is exploded on
/, so $piece will never contain a slash. And(/|$)is redundant AFAICT, because the+quantifier is greedy, so it will always match until the end of the of the string (or to a slash if there was one anyway).+ $path = str_replace($matches[1], $property, $info['view path']);If there are more %foo placeholders in $info['view path'], only the latter is replaced, because $path is overwritten inside the loop.
I suggest something like this instead (not tested):
foreach ($pieces as &$piece) {if (substr($piece, 0, 1) == '%') {
$piece = $entity->{substr($piece, 1)};
}
}
return implode('/', $pieces);
Also, some tests would be great.
#15
Imagefield just went in without this. While this looks good, it's probably Drupal 8 material at this point.
#16
imagefield's way of linking to the object is a temporary hack / workaround that works for nodes but not necessarily for other entities ($element['#object_type'] . '/' . $id). I don't think this can fly for D7.
#17
I agree this seems important, and I mostly agree with the comments in #14. But since code freeze is passed, we could actually do this in a much simpler way, by assuming the link is [view path]/[id] (untested):
<?phpfunction field_create_path($entity_type, $entity) {
$info = entity_get_info($entity_type);
list($id) = field_extract_ids($entity_type, $entity);
return $info['view path'] . '/' . $id;
}
?>
We keep the part of the patch that adds 'view path' to entity info.
If we actually do #606994: Move entity handling out of Field API, which is just renaming (or copying to preserve the API) field_extract_id() to entity_extract_id(), then this can be used system-wide.
#18
re #17: I do like the approach in #11, but if we feel pressed by time, we could also decide we don't support multiple replacements in the path pattern ?
#19
The only other thing this should support is an optional $vid
#20
Question for catch / bangpound:
In D6 we had taxonomy_term_path() and hook_term_path() : http://api.drupal.org/api/function/taxonomy_term_path
This does not fit with the "'URL to $object' as fixed pattern in hook_entity_info()" approach here, but I can't seem to find the equivalent in D7 - although we still have $vocabulary->module (which doesn't seem to be used anywhere now).
Was this removed on purpose in D7 ? Is this supposed to be doable from contrib ?
Can we think of other reasons why we should move to a CALLBACK_entity_url($object) approach instead ?
#21
Let's finally get this fixed.
Spending some more time going back and forth on this, I think a callback-based solution would be more flexible and future proof.
For instance, Barry worked on a 'combofield as fieldable field' approach some time ago. I'm not sure he went on with it, but this is a case where determining the path to generate for an imagefield 'as link' formatter needs some computation.
Attached patch does that. Should even be a tad faster.
#22
Can't find anything to complain about, we removed hook_term_path() in favour of hook_url_alter(), however this would be a more performant solution for modules wanting to recreate what we had in D6, since hook_url_alter() runs on everything. It's a small API change for modules which expose their own entity types, but these are very few, if at all, at the moment.
#23
Correct me if I'm wrong, but don't we lose functionality here? How does Taxonomy Rewrite module change the path returned by taxonomy entities with this approach?
#24
1) These are internal paths, not URLs - URL rewriting happens later.
2) A hypothetical taxonomy_entity_rewrite.module would implement hook_entity_info_alter() to replace the path callback function with its own.
#25
Ok, committed to HEAD. Thanks!
#26
Oops. Needs documentation.
#27
#28
Thks webchick.
I'm not sure I see what needs to be documented here. This doesn't affect D6->D7 module upgrade, AFAICT ?
#29
Holy crap, I love you all. This is a great addition to entity API. And a huuuuuge helper for Pathauto D7.
#30
Oh, I guess I was assuming somewhere we had some nice documentation about how to create entities, which would need to be adjusted to take this change into account.
HAHAHA I KILL ME. :(
Ok, setting back to fixed then.
#31
Oops, we're missing the path callback in user_entity_info().
#32
#33
Committed. But you know what that means, right? :P
#34
As discussed with catch in IRC, this approach does not fully support the case of the comment entity. The comment path callback value cannot be reused in the theme layer because it's a path internal to Drupal and it cannot include a fragment, as a result, in template_preprocess_comment() we end replicating (twice!) the logic of what comment_path() is supposed to do:
<?php$variables['title'] = l($comment->subject, 'comment/' . $comment->cid, array('fragment' => "comment-$comment->cid"));
$variables['permalink'] = l('#', 'comment/' . $comment->cid, array('fragment' => "comment-$comment->cid"));
?>
Also, the code above costs us an extra call to url() via l(): this is to be multiplied by 50 comments / page or more.We should allow the entity path callback functions to return a structure containing the path but also optional options compatible with url():<?php/**
* Entity path callback.
*/
function comment_path($comment) {
return array(
'path' => 'comment/' . $comment->cid,
'options' => array('fragment' => 'comment-' . $comment->cid),
);
}
?>
EDIT: ignore the extra call to l(), this will be dealt in a different issue, which will leverage the full support or url friendly entity path callback and make it cacheable by entitycache.
#35
and here is the patch
#36
Just in case 'path' isn't the first item in the array,
current(entity_path($obj_type, $object))take the array key directly instead of using current.#37
Reading #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6) might help to understand better what's the reason behind the patch #35.
@catch: I was trying to do that in one line but afaik PHP doesn't allow you to do that :(
#38
Agreed on the approach, I got "comment/$cid" wrong.
Just wondering: if the function returns an array with keys 'path' and 'options', is entity_path(), 'path callback', [entity_type]_path() terminology still appropriate ?
#39
@scor, yeah that's a two-line job :(
On terminology, hmm. Doesn't seem ideal, but can't think of anything better.
#40
the terminology needs to be changed, but could not find something appropriate. we need something broader than path. how about url_structure or url_elements?
or simply url? entity_url(), 'url callback', [entity_type]_url()
#41
Would url give the impression that it's a fully formed url, rather than a url() friendly array though?
#42
Yes, 'url' doesn't apply here.
Well, I don't think we have any name in core for the association of 'a drupal path + querystring and fragment options', so _path() might just be the best option.
#43
solving #36:
#44
+++ modules/image/image.field.inc@@ -461,7 +461,8 @@ function image_field_formatter_view($obj_type, $object, $field, $instance, $lang
- $path = entity_path($obj_type, $object);
+ $path_elements = entity_path($obj_type, $object);
+ $path = $path_elements['path'];
If entity_path() can have a 'path' and 'options', then the image formatter should take both into account, right ?
Else it won't work for linking to comments.
Powered by Dreditor.
#45
Also do we need to update the docs for hook_entity_info()? It currently says "path callback: A function taking an entity as argument and returning the path to the entity. "
#46
patch addressing #44 and #45.
#47
ignore patch above, this is the right one. I still think we need to have a better name than 'path' for the path and options elements, lines such as this are confusing:
<?php$path = $variables['path']['path'];
?>
#48
+++ modules/image/image.field.inc@@ -502,7 +502,12 @@ function theme_image_formatter($variables) {
+ $path = $variables['path']['path'];
+ $options = empty($path['path']['options']) ? array() : $path['path']['options'];
Hm, If I'm not mistaken, this should be
$path = $variables['path'];Then you have $path['path'] and $path['options'] ?
Optionally, the formatter could do
$path = entity_path($entity);
'#path' = $path['path'];
'#path_options' = $path['options'];
and let the theme function be a little simpler.
I also think entity_path() should always add an empty 'options' array(), if [entity]_path() callback provides none, for easier DX.
+++ modules/image/image.field.inc@@ -502,7 +502,12 @@ function theme_image_formatter($variables) {
+ $options = array_merge($options, array('html' => TRUE));
why not simply
$options['html'] = TRUE;?Powered by Dreditor.
#49
The key '#path_options' in image_field_formatter_view() does not get passed to theme_image_formatter().
I've made the path 'options' key default to array().
#50
Discussed this with catch on IRC. The 'path' term is too specific here. I'd like to suggest the broader term 'uri' which includes the 'path' and the various 'options' like fragment and query which can be used to build the uri of an entity (comment entities have a fragment in their uri for example).
uri callback: A function taking an entity as argument and returning the uri elements of the entity, e.g. 'path' and 'options'. The actual entity uri can be constructed by passing these elements to url().The '#path' key in the formatter would need to be aligned with the fact that we can have a uri instead of a path only.
#51
just realizing I forgot to attach the patch in the previous comment... :/
#52
The last submitted patch, 525622_comment_path_50.patch, failed testing.
#53
#51: 525622_comment_path_50.patch queued for re-testing.
#54
This looks great. I can't think of anything other than 'uri' which describes this somewhat accurately, and the url() friendly array is something we've done in a lot of functions. It's a teensy API change, but the path callback only went in a couple of weeks ago, isn't documented yet, there's barely any entity modules floating around, and it qualifies as a proper bug since it's actually the wrong thing for comments in HEAD - so we should get this in quick and /then/ document it.
#55
Subcribing.
#56
Committed to HEAD.
#57
Oooh, thank you. Back to cnw for docs.
#58
Follow-up issue: #823428: Impossible to alter node URLs efficiently
#59
For people who worked on this issue, I would really appreciate your input on #823428: Impossible to alter node URLs efficiently. Dries and chx are questioning that patch, and therefore, I'm asking the community for how to make sense of the entity_uri() function. Thanks!
#60
reverting to the original issue title, for later reference
#61
#62
As per #57 - can you clarify what docs need to be written?
If it's an entry in the module/theme update guide that's needed, please change the tag to "Needs Update Documentation". If it's something else, you can leave the tag at "Needs Documentation", but please explain what is needed.