Posted by Deciphered on January 20, 2010 at 11:24pm
Jump to:
| Project: | Node displays |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | support request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Hi,
I'm looking at adding support for ND CCK to one of my modules, and I'm currently playing around with adding formatters to fields but I discovered a potential issue.
Using hook_nd_fields_alter() to add formatters to the fields I noticed that if I add one formatter to a field that has no formatters defined ('read_more') it will default to the new formatter, whereas the default 'code' behavior of the field should really be a choice, especially if a user installs a module that does what I'm doing and they don't like the modules defined formatter.
Wouldn't it make more sense for the 'code' value to be a 'formatter'?
Maybe I'm off the mark here, I have only been looking at the code for a few minutes so far.
Cheers,
Deciphered.
Comments
#1
PS. please ignore my reference to ND CCK, I was originally going to post the issue there until I realized the issue is with Node displays not ND CCK.
#2
I also note that adding formatters to a field is not enough, as you have to change the field type to ND_FIELD_THEME to even allow the use of a formatter.
I guess at this stage it doesn't really look like Node displays is engineered to do what I'm trying to do.
The basics of what I want is to allow my Custom Formatters module to be able to create formatters that can be attached to any of your Node displays fields.
I would be happy to hear any thoughts you have on this issue.
Cheers,
Deciphered.
#3
Hi, I've been looking at your module to work together because we want also to add custom formatters. Our target for that is for 1.1 which will only see the light in two months. I'll get back to this after we release 1.0!
#4
Sounds good to me.
#5
Ok, small update. I'm experimenting a bit with changing the array format of fields (not committed yet). All 'extra' properties that a field can have (block, code, formatters, render etc) are moved to a key called 'properties'. The 'read more' field in ND looks like this now:
<?php'read_more' => array(
'title' => t('Read more'),
'type' => DS_FIELD_TYPE_CODE,
'status' => DS_FIELD_STATUS_DEFAULT,
'properties' => array(
'formatters' => array(
'ds_eval_code' => t('Default'),
),
'code' => '<?php echo l(t("Read more"), "node/$object->nid"); ?>'
),
),
?>
When I do this, the field gets a selection box on the display screen, so that's good.
I changed the code in DS where DS_FIELD_TYPE_CODE is executed to this:
<?php$format = (isset($field_settings[$key]['format'])) ? $field_settings[$key]['format'] : 'ds_eval_code';
if ($format == 'ds_eval_code') {
ds_eval_code($key, $field, $object, $object_type);
}
else {
$key = $key .'_rendered';
$object->{$key} = theme($format, $object);
}
?>
With this change, it should be fairly easy to implement hook_ds_fields_alter, look for DS_FIELD_TYPE_CODE and add extra formatters I think. Let me know you think! We're pretty close to a release, and moving the formatters also to the properties key looks like a real good move (and all my ND tests still pass!). I'd rather do that now instead in 1 or 2 releases where we might break a lot of implementations, which we all don't want :)
#6
Note, without testing, it should also work on fields of type DS_FIELD_TYPE_THEME eg title in nd:
<?php'title' => array(
'title' => t('Title'),
'type' => DS_FIELD_TYPE_THEME,
'status' => DS_FIELD_STATUS_STATIC,
'properties' => array(
'formatters' => array(
'nd_title_h1_nolink' => t('H1 title'),
'nd_title_h1_link' => t('H1 title, linked to node'),
'nd_title_h2_nolink' => t('H2 title'),
'nd_title_h2_link' => t('H2 title, linked to node'),
'nd_title_h2_block_nolink' => t('H2 block title'),
'nd_title_h2_block_link' => t('H2 block title, linked to node'),
'nd_title_p_nolink' => t('Paragraph title'),
'nd_title_p_link' => t('Paragraph title, linked to node'),
),
)
),
?>
Edit: added php code of title field.
#7
Looks promising.
I did put your code into my versions, and one quick thing I noticed was that the select box was still looking at the old array position. Simple fix to change:
Change line #195 of ds/includes/ds.display.inc from:
$form[$field][$build_mode]['format']['#options'] = isset($value['formatters']) ? $value['formatters'] : array();To:
$form[$field][$build_mode]['format']['#options'] = isset($value['properties']['formatters']) ? $value['properties']['formatters'] : array();I'm sure this isn't the only issue, and I'm sure you didn't post all the code changes you've made, but I thought I should post this just in case.
Looking forward to where this is going.
Cheers,
Deciphered.
#8
Hi swentel,
I now have a proof of concept version of Custom Formatters that has the ability to make custom formatters available for use with ND (with the alterations as above).
My tests so far have been successful, but based on the approach I had to take I'm sure there will be a few issues. The main problem was that the $object being themed was just a node object, with no way of indicating which particular field was being themed, so I based it as the last key in $object->content as the current field, which may or may not always be correct.
Cheers,
Deciphered.
#9
One issue I just noticed is that when attempting to create a custom formatter for one of your DS_FIELD_TYPE_THEME type, such as title, the theme functions are setting the $node object instead of returning a value.
My previous test that worked was on the 'read_more' field, which DS calls as:
$key = $key .'_rendered';$object->{$key} = theme($format, $object);
Opposed to this way:
theme($format, $object);This is definitely a problem for my side of things. It would work if DS_FIELD_TYPE_THEME did things the same way as DS_FIELD_TYPE_CODE, but I'll leave that to you.
Cheers,
Deciphered.
Edit: Made that change myself and it worked perfectly.
#10
Ok, I committed my changes to ds, nd and cd. The line in ds.display.inc was indeed on my localhost, now in CVS too. You're right, there are a few issues still how DS_FIELD_TYPE_THEME works. I'm going to think about this today. I'll be on IRC also today, not sure if you're on there too. Ping me. I'd love to see some code from CF to see how you're adding new formatters too - I just did a small test
I've also added this issue as the last blocker for a release 1.0 so, I'm excited, to get this working nicely, for both!
Edit: CF spits a lot of notices, I'll provide a patch once we get this once cleared too!
#11
Yeah, I know about the notices, they are the side effect of the way I'm checking if there the CCK fields are empty or not. The feature had to be disabled to make the ND/DS stuff work anyway, so the notices should no longer be an issue (local version).
#12
Almost there! Changes I've commited to DS and ND:
I played around with theme_custom_formatters_formatter. What still gives me troubles is the _custom_formatters_token_replace(). Commented out, any advanced custom formatter still works fine and all other fields are themed. I also made the function a bit smaller, making use of drupal_eval which makes it a bit more readable I think. Of course, it's up to you what you're doing with it :)
<?php
function theme_custom_formatters_formatter($element) {
// @todo Add back the formatted values in the 'view' element.
// @see content_field('alter').
//content_field('alter', $element['#node'], array('field_name' => $element['#field_name']), $items = array($element['#item']), '', '');
$output = '';
// Give modules a chance to alter the formatter element.
drupal_alter('custom_formatters_formatter_element', $element);
$formatter = is_object($element['#formatter']) ? $element['#formatter'] : custom_formatters_formatter_by_name($element['#formatter']);
switch ($formatter->mode) {
case 'basic':
//$output = _custom_formatters_token_replace($formatter, $element);
break;
case 'advanced':
$output = drupal_eval($formatter->code);
break;
}
return $output;
}
?>
I've changed nd_custom_formatters_formatter_element_alter to this:
<?php
function nd_custom_formatters_formatter_element_alter(&$element) {
if (isset($element['module']) && $element['module'] == 'nd') {
$formatter = $element['formatter'];
// Rebuild element.
$element = array(
'#formatter' => drupal_substr($formatter, 28),
'#node' => $element['object'],
'#type_name' => $element['type'],
'#field_name' => $element['key'],
'#weight' => NULL,
'#theme' => $formatter,
'#item' => (array) $element,
);
}
}
?>
The only problem left is to find out why _custom_formatters_token_replace is making a lot of stuff dissapear.
#13
Ok, with my code for theme_custom_formatters_formatter , adv mode stopped working on CCK - checking out later.
#14
Hi swentel,
The reason I didn't use drupal_eval() is that I needed to pass the $element variable to the eval code, which is why your version is no longer working properly.
As for the other things, looking through it all now.
Cheers,
Deciphered.
#15
So I grabbed your latest code from CVS, and going from the version I sent you the main change to make things work is:
Change line #351 of custom_formatters.module from:
unset($element['#node']->content);To:
if (isset($element['#node']->content)) {// If the following is not done, basic formatters will cause all preceding
// fields to not be formatted. The code is completely illogical (IMHO), but
// it works.
$node_content = $element['#node']->content;
unset($element['#node']->content);
$element['#node']->content = $node_content;
}
As documented in the code, it is a crazy piece of code, but if you don't do it basic formatters break things. And as you noted, if you completely remove $element['#node']->content then ND formatters break. So the code is deleting the value then adding it straigh back, and for some reason that works.... odd, but it works.
I also noted that ND CCK is going to need to be updated to the changes you have made. I realize it's not your module, but I figured you might want to alert the developer once the changes have been finalized.
Cheers,
Deciphered.
#16
Yep, insane piece of code, but seems to work strange enough.
I have full control over ND CCK - maintainer is a colleague, but I have CVS access. I'm not sure what I should change there since tests here seem to be going fine - besides a lot of PHP notices still. Theming of CCK fields doesn't run through DS anyway. However, I'll look into that tomorrow or friday, depending my workload at work. If you see something that need to changed, let me know. Thanks for the effort already, much appreciated, this already rocks a lot imo :)
#17
I committed an update to CF, the majority, if not all of the notices are gone now.
I will have a look at ND CCK when I can, but I think the only change should be the array structure (properties/formatters).
Cheers,
Deciphered.
#18
Properties format is ok, see http://drupalcode.org/viewvc/drupal/contributions/modules/nd_contrib/nd_...
#19
Ok, one notice remains:
Notice: Undefined index: filepath in custom_formatters_token_values() (line 53 of /var/www/html/drupal/drupal6/sites/all/modules/custom_formatters/modules/token.inc).
a simple isset check on $file['filepath'] removes the notice.
<?phpif (isset($file['filepath'])) {
$values['filefield-imagecache-' . $preset['presetname']] = file_directory_path() . '/imagecache/' . $preset['presetname'] . str_replace(file_directory_path(), '', $file['filepath']);
}
?>
I think we can mark this fixed than, this is great, thanks! I can now throw out a very cool release - I'll also include some examples on my demo site (http://demo.customsource.be) soon!
#20
Automatically closed -- issue fixed for 2 weeks with no activity.