Posted by yched on February 1, 2009 at 3:14pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | normal |
| Assigned: | zzolo |
| Status: | closed (fixed) |
Issue Summary
from moshe in http://drupal.org/node/361683#comment-1231951:
This should be a drupal_alter() to be consistent with similar hooks on the node and page level: foreach (module_implements('field_attach_view') as $module) {. I know that this would be slightly inconsistent with other field attach hooks, but thats how we usually handle the view operation.
Comments
#1
As per the field initiative, I'd like to give this task a whirl.
#2
Great!
#3
So, I changed the ~hook to a drupal_alter. I am going to describe what I did, just to make it easier to review as I not entirely sure what this issue is encompasses.
field.api.php documentation:
<?phpfunction hook_field_attach_view_alter(&$output, $obj_type, $object, $teaser) {
}
?>
field.attach.inc function:
<?php
function _field_attach_view($obj_type, &$object, $teaser = FALSE) {
// Let field modules sanitize their data for output.
_field_invoke('sanitize', $obj_type, $object);
$output = _field_invoke_default('view', $obj_type, $object, $teaser);
// Let other modules make changes after rendering the view.
drupal_alter('field_attach_view', &$output, $obj_type, $object, $teaser);
return $output;
}
?>
From looking at all the attach hooks, should they all be turned into drupal_alters.
_field_attach_*Also, this is my first core patch so please feel free to be thorough in any criticism.
#4
@zzolo: Thanks for your patch!
When you submit a core patch that you think is ready, change the status to 'needs review'; this encourages people to look at it whereas an 'active' issue generally looks like "no one is working on it yet" so it gets ignored. Also, 'needs review' also triggers the test bot on the patch. It's totally okay (and more common than not) for patches to go from 'needs review' to 'needs work' lots of times, so don't worry if you think the patch might not be 100% perfect yet.
#5
@bjaspan, thanks. I thought I had changed the status, but apparently not.
So, if you have a moment, can you review this and let me know if I am on the right track and if I shoudl make all the attach hooks like this. Thanks.
#6
Looks good to me. Two minor nits:
+ * @param &$output. Remove the leading ampersand. We don't keep that in our doxygen+ drupal_alter('field_attach_view', &$output, $obj_type, $object, $teaser);. We don't need the ampsersand here either. The alter hook implementations will specify by ref instead. AFAIK, the form of by ref in this patch throws a 'call be reference' notice and is not allowed.To answer your question - this is the only hook that needs to convert to drupal_alter(). We do that to maintain consistency wigth other 'view' alterations in node, user, etc.
#7
Thanks, Moshe. Stupid mistake on the ampersand in the function call. Similar patch plus Moshe's suggestions.
#8
Looks good.
#9
Committed to CVS HEAD. Thanks!
#10
Automatically closed -- issue fixed for 2 weeks with no activity.