| Project: | Content Construction Kit (CCK) |
| Version: | 6.x-2.x-dev |
| Component: | Views Integration |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
I have a custom CCK field that pulls information tied to the user's inputted ID from an external REST API. This info is cached by Drupal/CCK's caching system and only loaded each time hook_field('load') is called for my field module. So it adds other stuff to the field so it has something like this...
$node->field_foo[0] = array(
'value' => 'someidinsertedbyuser',
'extra' => 'just loaded this over REST from hook_field(load)',
);
Can I not expose these dynamic values to views in hook_field_settings('tables') since I don't have dedicated DB fields for them? I avoided doing this as these need to be invalidated every so ofter and leveraging Drupal's caching system was a great way to do this cleanly as all this gets serialized in the cache table anyways.
Any thoughts would be appreciated.
Thanks,
-Rob
Comments
#1
Not sure I get this.
CCK by default only exposes the tables / columns it handles itself, (
hook_field_settings('database columns')).If you're adding external data on field load, they should be ignored by views.
Can you give more info on exactly what you're expecting ?
#2
I think I'll just need to create columns for these dynamic fields since they are only stored in CCK's cache_content table and views couldn't really do direct joins. I was thinking there could be an easy way out so I wouldn't have to write extra code to invalidate my own field's cache and just piggyback on any cache_clear_all that invalidates my node (forcing hook_field('load') to do a REST call to update the data).
I'm probably not explaining this well, but I think this is a by design. Thanks for the response yched!
#3
@yched, Okay, I've create db columns for my extra field data...
<?phpcase 'database columns':
$columns = array(
'value' => array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => "''", 'sortable' => TRUE),
'widget_uid_to_show' => array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => "''", 'sortable' => TRUE),
'widgets' => array('type' => 'longtext', 'not null' => TRUE, 'default' => "''", 'sortable' => FALSE),
'goal' => array('type' => 'integer', 'not null' => TRUE, 'default' => '0', 'sortable' => TRUE),
'start_at' => array('type' => 'integer', 'not null' => FALSE, 'default' => NULL, 'sortable' => TRUE),
'end_at' => array('type' => 'integer', 'not null' => FALSE, 'default' => NULL, 'sortable' => TRUE),
'click_url' => array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => "''", 'sortable' => TRUE),
'external_count' => array('type' => 'integer', 'not null' => TRUE, 'default' => '0', 'sortable' => TRUE),
'external_total' => array('type' => 'integer', 'not null' => TRUE, 'default' => '0', 'sortable' => TRUE),
'transaction_count' => array('type' => 'integer', 'not null' => TRUE, 'default' => '0', 'sortable' => TRUE),
'transaction_total' => array('type' => 'integer', 'not null' => TRUE, 'default' => '0', 'sortable' => TRUE),
'location_count' => array('type' => 'integer', 'not null' => TRUE, 'default' => '0', 'sortable' => TRUE),
);
return $columns;
?>
That is working fine, but I wanted to expose these to Views primarily so I can sort on them (filters, args would be nice too). I thought setting 'sortable' => TRUE would allow that, but I still only get the 'value' field. I noticed that content_views_field_tables() does an
<?php$main_column = array_shift($columns);
?>
so it only gets the 'value' field while putting the other fields in 'addlfields'. I tried to suss that out, but couldn't really see what that got me. I'd like to expose all these columns to Views just like the first one, but returning an array of all them (not just the 'value') didn't work. Any thoughts? TIA
#4
I realized when modifying content_views_field_tables() I didn't take those $bales['sorts'] = array(); out of the loop so they were resetting. This appears to be working okay so far! I'm stoked. So this is a feature request now. Will roll a patch but this needs some feedback as it's still rough I'm sure, just wanted to get this up. Should we maybe do a conditional, if a db column is sortable then expose the full column to Views instead of just the first. Or provide 'filterable', etc. to see what should be exposed to Views and in what extent?
Or, is this too particular to my case? I think imagefield and others could use this...sorting by the "title" or "alt" fields. That's a lame example, but you get the idea.
<?php
function content_views_field_tables($field) {
$field_types = _content_field_types();
$db_info = content_database_info($field);
if (count($db_info['columns'])) {
$table = array();
$table['name'] = $db_info['table'];
$table['join'] = array(
'left' => array(
'table' => 'node',
'field' => 'vid',
),
'right' => array(
'field' => 'vid',
),
);
$module = $field_types[$field['type']]['module'];
$formatters = array();
if (is_array($field_types[$field['type']]['formatters'])) {
foreach ($field_types[$field['type']]['formatters'] as $name => $info) {
$formatters[$name] = t($info['label']);
}
}
$columns = $db_info['columns'];
$table['fields'] = array();
$table['sorts'] = array();
$table['filters'] = array();
foreach ($columns as $column => $attributes) {
$table['fields'][$columns[$column]['column']] = array(
'name' => $field_types[$field['type']]['label'] .': '. $field['widget']['label'] .' ('. $field['field_name'] .')',
'sortable' => isset($columns[$column]['sortable']) ? $columns[$column]['sortable'] : FALSE,
'query_handler' => 'content_views_field_query_handler',
'handler' => array('content_views_field_handler_group' => t('Group multiple values'),
'content_views_field_handler_ungroup' => t('Do not group multiple values')),
'option' => array('#type' => 'select', '#options' => $formatters),
'content_db_info' => $db_info,
'content_field' => $field,
'content_field_module' => $module,
);
if (isset($columns[$column]['sortable']) && $columns[$column]['sortable']) {
$table['sorts'][$columns[$column]['column']] = array(
'name' => $field_types[$field['type']]['label'] .': '. $field['widget']['label'] .' ('. $field['field_name'] .','. $columns[$column]['column'] .')',
'field' => $columns[$column]['column'],
'content_db_info' => $db_info,
'content_field' => $field,
'content_field_module' => $module,
);
}
$filters = module_invoke($module, 'field_settings', 'filters', $field);
if (is_array($filters) && count($filters)) {
foreach ($filters as $key => $filter) {
$filter_name = '';
if (count($filters) > 1) {
$filter_name = (!empty($filter['name'])) ? $filter['name'] : $key;
$filter_name = ' - '.$filter_name;
}
$name = $field_types[$field['type']]['label'] .': '. $field['widget']['label'] . $filter_name .' ('. $field['field_name'] .','. $columns[$column]['column'] .')';
$init = array(
'name' => $name,
'field' => $columns[$column]['column'],
'content_db_info' => $db_info,
'content_field' => $field,
'content_field_module' => $module,
);
$table['filters'][$columns[$column]['column'] .'_'. $key] = array_merge($filter, $init);
}
}
}
// We don't use $db_info['table'] for the key, since that may change during
// the lifetime of the field and we don't want to require users to redefine
// their views.
return array('node_data_'. $field['field_name'] => $table);
}
}
?>
#5
@yched, this is a question we should consider. The default handling only picks up the first column defined by a field, but should we have the core views handling automatically add in all columns? I have already had to write some custom Views handling in a couple modules just to get additional columns into Views, so there are use cases for doing that. The problem is the code bloat and the sheer number of fields that this may create in Views, so I'm not sure if this should automatically be done or not. The alternative is to do what we do now - expose only the first column automatically, and then force modules to do their own custom handling for other columns.
@RobRoy, there is a new method for handling custom views in custom modules, so if we don't add this to core CCK views handling you can still do that. Basically, in your module you add a 'tables' operation to your field settings that will return your Views tables (similar to the 'filters' operation). Core CCK will take whatever array you return instead of creating its own tables. You can use the code you illustrate here to create the tables or call content_views_field_tables($field) to get the tables created by core CCK as a starting point, then add to them whatever you need to add before returning them.
#6
I think we do not want to expose _all_ columns to Views by default - at least as long as Views UI sticks all the fields / filters / sorts in a dropdown.
But RobRoy's suggestion sounds good, expose for 'sorts' the columns that are set 'sortable', etc...
Problem is we already have an extended facility for custom filters (
hook_field_settings('filters')), which might not be consistent with that.#7
@KarenS, thanks. I'm actually already calling mymodule_views_field_tables() from hook_field_settings('tables') to stay out of CCK core for now. I just renamed this function to content_views_field_tables() for the sake of example and in case someone wanted to plop it in and test it.
I've been tweaking this a little bit and think that it wouldn't be too difficult to add args for 'filterable' and 'fieldable' (In English please?) then choose what to expose to Views by looking at those 3 values in content_views_field_tables().
@yched, I was thinking about the 'filters' problem, but can't wrap my head around when that would cause problems. Could you provide an example? I see content_views_field_tables() invokes 'filters'...
#8
I think what yched is referring to is that the way that 'filters' was set up is inconsistent with the way we're handling other things, so we need to maybe rethink that.
Anyway, I too am leaning toward the idea of adding a flag to indicate which fields should get their own views fields. We don't need to add them all. The text module has a column for the filter format, but that doesn't need to be a views field. I think the length of the list in Views is irrelvant, or maybe should be created as a Views issue. We should focus here on exposing anything that ought to be exposed without trying to decide what is 'too much'. If we don't show all the columns and I just end up using custom code to get them in there, I still end up with the same number of items in Views, it just took more work to get there.
So what would you call it -- indeed 'fieldable' is not right. It may need to be two words, like 'views field' or 'in views'.
#9
views_filter => TRUE
views_sort => TRUE
that sort of naming feels about right
#10
Has this gotten anywhere since the last post? I am finally getting around to fixing my cck modules to work with views as currently stands, but as was mentioned in another issue thread, my cck_fullname and cck_address modules have multiple fields that should be sortable.
CCK rules!
#11
Bump - would still like to see this.
#12
Hello,
Here's a patch built for D6. I tested it with CCK Money for D6 which for the moment relies on the default CCK implementation, and this seems to work perfectly for us.
I see that this is a D5, but I thought I could post this here. Please, let me know if you prefer a separate issue. Thanks! :)
#13
#14
#15
#16
Ok, this will indeed not happen in D5. We won't make such big changes in the D5 branch now.
A few remarks :
- There's no need to provide 'fields' for all columns - the one 'field' that already exists retrieves all columns and display is done through formatters, which do whatever they need to do with the full data.
- For other entities (filters, args, sorts), my main concern is overcrowded UI, losing the users between many choices that, in many cases, will be of little interest.
text 'format' ?
filefield 'list' ? filefield 'data' (serialized array of... I don't exactly know what) ??
+ filefield, for instance, implements no filters of its own (defers to generic file filters trough a relationship on the files table)
AFAIK date module completely bypasses the default Views integration and builds its own.
I'm not sure imagefield is settled wrt Views integration.
All in all, my personal feeling is that it's really a matter of choice by each field type. I'm currently completely not convinced we should provide this by default.
+ Adding this now will mean that all field types that do *not* want those additional filters/args/sorts will need to be updated, leading to many sorts of maintainance headaches wrt versions combinations (new CCK + old field module = you get some filters that will disappear and break your views when you upgrade the field module)
IMO it's really
#17
How about something like hook_field_settings('views columns') that simply expects the names of the columns a CCK field wishes to expose to views? The expected result would be similar to hook_field_settings('save')
That way, CCK fields that implement that, can tell CCK which DB columns implemented by the CCK field module can be exposed to views "by default". And it will not affect existing CCK field implementations. :)
#18
If implementing hook_field_settings('views columns') was a good idea I could re-roll the patch in #13.
I think it's an approach that would not affect existing CCK field implementations. By default, if hook_field_settings('views columns') was not implemented, the default behavior of CCK would remain unchanged.
For example, Money CCK could implement hook_field_settings('views columns') this way:
<?phpcase 'views columns':
return array('amount', 'currency');
?>
And that could be the trigger for CCK to implement default views integration for all field columns in this case. Other modules would not be affected by this change in CCK.
#19
Could work. Pondering a little on the best way to go :-)
#20
hmm... here's another possible approach. Instead of implementing hook_field_settings('views columns'), we could extend the data expected by hook_field_settings('database columns').
<?phpcase 'database columns':
return array(
'amount' => array('type' => 'numeric', ..., 'sortable' => TRUE, 'views' => TRUE),
'currency' => array('type' => 'varchar', ..., 'sortable' => TRUE, 'views' => TRUE),
);
?>
The new thing is
'views' => TRUE. By default, it would be FALSE, except for the first column, so that existing CCK fields would not be affected.#21
Attached patch works as decribed in #20
When
'views' => TRUEis not used, thencontent_views_field_views_data()picks up the first column defined for the field (backwars compatibility with existing fields).Please, critique! :)
#22
Hi, here's yet another approach :)
The idea is that it would be coded like in #21, but instead of getting the list of columns from 'database columns' array (the 'views' => TRUE I described in #20), it would get the list from a second optional argument to function
content_views_field_views_data().So a CCK field module such as Money could do something like this:
<?phpcase 'views data':
return content_views_field_views_data($field, array('amount', 'currency'));
?>
Should I roll a patch with this one?
#23
wow, thks Markus - need a little time to ponder :-)
[edit : LOL, I posted about the same follow-up 3 comments above. I do, though ;-) ]
#24
Not so fond about #22. I think I could go with either #18 or #20. Karen, do you have a preference ?
I think we should have the 'expose 1st column if nothing specified' behavior for a release or two to let field modules some time to update, and remove it at some point (no views integration if no column explicitly exposed). The less 'special cases' the better.
Detail : it's important IMO that 'title' contains both label and field_name. Most users know their nodes by their label, but different fields can have the same label and a given field can have different labels in different content types (we just pick one for the 'title')
#25
Please note the way 'title' is built has not been in the patches I posted. It still contains the widget label and field name.
What I changed is 'title_short' which was just the widget label, and I added the column name, which is necessary to see which form element pertains to which column of the CCK field.
For both, the widget label is still wrapped in t(), but not the field name nor the column name.
#26
- 'title' => t('@label - (!name)', array('@label' => t($field['widget']['label']), '!name' => $field['field_name'])),+ 'title' => t('@label - (!name)', array('@label' => t($field['widget']['label']), '!name' => $db_field)),
?
#27
Oops! I was wrong in my explanation in #25. Sorry. :(
What I did is using
$db_fieldrather than$field['field_name']. The difference is the former uses the column name, while the later uses the CCK field name.If you create a Money CCK field with a name "field_money" and a label "Money", then with original code, you get
$field['field_name']equal to "field_money", which is fine when only one column is exposed to views."title"would be set to "Money (field_money)", and"title_short"would be set to "Money". But if you expose more than one column to views, then you end up with 2 fields with the same label. One of them would really be "amount" column, and the other one would really be "currency" column. No hint for the user to know which is what.On the other hand, using the column name to build the views field label, as I did in the patch, you would get 2 fields in views labeled like this: "Money (field_money_amount)" and "Money (field_money_currency)". And
"title_short"would look like: "Money (amount)" and "Money (currency)".That's what I really did in the patch above. Sorry for the confusion in #25.
#28
#24 - I think we cannot change the API at this point, so I don't think we can drop the 'expose first column' behavior, or at least I think that is important enough change for a point release. We can't risk breaking Views integration for thousands of sites that don't keep up with all their updates. I think we should just use a method where exposing the first column is the default method always unless something else is specified.
I'm neutral on all the ways to specify additional columns, so long as the default behavior is the first column. I can't really see any big advantages or disadvantages of any of them. I think #20 is the way I'd always assumed we would do it.
This is a very nice addition, thanks!
#29
One caveat with the approach in #20, extending
hook_field_settings('database columns'), is that upgrading CCK fields will require an update of the "db_columns" field in table {content_node_field}, and also updating the content_cache stuff. :(The approach in #18, using
hook_field_settings('views columns'), doesn't, but it looks hack-ish, IMO,The approach in #22, adds an optional argument to
content_views_field_views_data(), which gives more freedom, and still doesn't affect existing CCK field modules.#30
Actually, as I look at it more, I think I agree with yched that I'm not crazy about adding parameters to content_views_field_views_data() -- that's a slippery slope that ends up with cobbled up functions with long lists of optional arrays of data.
Upgrading the field data is not a big deal really, and we'll need an update function anyway to clear the views cache so the new values will become available. And it might actually end up being an advantage to have that info in the $field array -- no particular use case now, but you could easily access that information in custom Views handlers.
#31
Then... the patch to review is the one in #21.
Any hint on how to write
hook_update_N()to upgrade CCK fields that take advantage of'views' => TRUEextension tohook_field_settings('database columns')? :-|#32
Look at content_update_6009() which was updating the widget settings array() and do something similar to update the field columns. And note that when you're updating serialized values you have to use db_query() instead of update_sql() because update_sql() clobbers the serialized value.
#33
Agreed on going with #20/#21. It has been discussed that Views data might eventually belong to the db schema description, and that's exactly where #21 puts it.
About names : I'd rather go with
- title : "Label (field_name) - short_column_name" : "Price (field_price) - currency"
except when there is only one column exposed, where we'd keep the existent "Label (field_name)"
- short : "Label_truncated - short_column_name", where label is truncated to, say, 8 or 10 : "Price - currency"
except when there is only one column exposed, where we'd keep only "Label_truncated"
#34
Hm, which will make translators want to localize 'currency'...
This might call for an additional 'column readable name' property in hook_field_settings('database columns'). We don't *need* to add that now, though.
I still prefer that to displaying mangled field name and column (field_price_currency) as #21 currently does.
#35
I'm not sure if I get it. Do you mean it should be done like this?
<?php'title' => t('@label (!name) - !column', array(
'@label' => t($field['widget']['label']), // Widget label, which is translated here, btw.
'!name' => $field['field_name'], // Field name. ie. 'field_money'.
'!column' => $columns[$i], // Short column name. ie. 'amount' or 'currency'.
))
?>
...or maybe we could do like this?
<?php'title' => check_plain(t($field['widget']['label'])) .' ('. $field['field_name'] .') - '. $columns[$i],
?>
¿?
#36
Right - 1st option, to let translators handle their own language specific punctuation rules around - and ().
+ some logic to show '- !column' only if there are several exposed columns.
#37
There we go. :)
A few notes on the attached patch:
a) Building label truncated: I've used the following. ie. Truncate the translated label, cut in word boundaries and do not add '...'.
<?php$label_truncated = truncate_utf8(t($field['widget']['label']), 10, TRUE);
?>
b) Building short version of the title:
- I'm using 'title short' rather than 'title_short' to keep this in sync with: #326034: Short handler titles in admin summaries
c) The rest of the code is identical to what we've seen in #20/#21. ie. we're extending
hook_field_settings('database columns')with'views' => TRUE(FALSE by default, so it doesn't affect existing CCK fields).#38
As KarenS suggested in #32, I have written the hook_update_N() for the Money CCK field. Can someone check if it looks correct, please?
It worked for me on my development environment, it's just I'm not sure if I should do something else.
<?php
/**
* Update all money fields to reflect the new Views integration feature.
*
* References:
* http ://drupal.org/node/131953 (CCK feature request)
* http ://drupal.org/node/335546 (Money CCK field task)
*/
function money_update_6000() {
drupal_load('module', 'content');
if ($abort = content_check_update('money')) {
return $abort;
}
$ret = array();
$result = db_query("SELECT field_name, db_columns FROM {content_node_field} WHERE type = 'money'");
while ($field = db_fetch_object($result)) {
$db_columns = unserialize($field->db_columns);
if (empty($db_columns['amount']['views'])) {
$db_columns['amount']['views'] = TRUE;
$db_columns['currency']['views'] = TRUE;
$success = db_query("UPDATE {content_node_field} SET db_columns = '%s' WHERE field_name = '%s'", serialize($db_columns), $field->field_name);
$ret[] = array(
'success' => $success !== FALSE,
'query' => strtr('Updating definition of the Money CCK field: %field-name.', array('%field-name' => $field->field_name)),
);
}
}
if (!empty($ret)) {
content_clear_type_cache();
}
return $ret;
}
?>
If it's correct, then I hope it can be of help to someone else that may think about using this feature for their CCK fields. :)
Edit: Updated the code to reflect the version I committed to the Money CCK field module.
#39
This looks fine except that since you're using a content function you need to include content.module (see that we do that in all updates now) because sometimes it isn't available during updates. Other than that, it looks like the right way to do it.
#40
@KarenS: Thank you, and sorry for the off-topic.
I just noticed that
content_associate_fields(), which is defined incontent.module, could be invoked from withincontent_check_update(), butnumber_update_6000()loads the content module aftercontent_check_update()is invoked, so there might be situations wherecontent_associate_fields()could not be there? ...or maybecontent_check_update(), which is located incontent.install, could rundrupal_load('module', 'content')sohook_update_N()implementations in CCK fields don't need to do it themselves ?#41
You think writing some thing like this for gmap or location module would help with my issue http://drupal.org/node/338300 ... Where viewsis not getting values from a cck location fields?
#42
Just in case this has been missed. There's a patch pending review at #37.
Thank you :)
#43
Markus, is there a reason why you declare a (Views-)'field' for all columns ?
In #16 I wrote :There's no need to provide 'fields' for all columns - the one 'field' that already exists retrieves all columns and display is done through formatters, which do whatever they need to do with the full data.
The 'fields' for column1 and for column2 will have the exact same behavior and output, so exposing one per column is no good IMO and will confuse users.
#44
hmm... yes, the field definition is redundant, but we need to separate arguments, filters and sort options. These are separate fields in the database.
So, maybe I could roll a patch where the field is defined just once, but arguments, filters and sort options are per field column? Make sense this approach?
#45
markus : nevermind, I adapted the code already. Patch attached.
I'm short on time for a real test and review, though. I'll try again tomorrow, and will have to defer to after the 'fields in core sprint' if I fail :-/
#46
Doh - Patch.
#47
Thanks! :-D
I'll test this later and see how it works here. We have a lot of CCK content types with a bunch of different fields.
#48
Tested and worked like a charm. Thank you :)
This will help CCK fields like Money much easier. But also when one implements custom CCK fields with more than one DB column per particular site requests.
BTW: Have a nice trip to Acquia! :)
#49
Oops! There was a problem caused by 'additional fields' built with only the column that was been explicitly enabled, while it should contain all field related columns.
The attached patch fixes this.
#50
OK, I can't say I extensively tested, but it seems to do what it's supposed to without major break, so, committed.
Thks for your patience, Markus (er, you'll probably still need some more for the other patches :-/ )
#51
Thank you very much for taking the time to review and commit this. It's much appreciated what you and Karen do.
#52
BTW, if text field has been updated to use this feature, it may need a new hook_update_N() similar to the one I implemented in Money. Example code in #38 ¿? If so, I would be happy to write the patch.
#53
Discarded. Issue is too old.