Hi,

I created a patch for this module which doesn't display the tab if there's no actual content in it. Right now it checks for text, but that should be appropriate for most cases.

Please test it if you have time and let me know if it's a good solution.

Thanks,

Rob

Comments

verikami’s picture

...it works fine, but...

a) it displays the tab when field contains php code but it returns nothing
b) tab containing only referenced node(s) and without html is not displaying

I have such cases ;-) so :-(

...propably it could check rather the final output...

mikeytown2’s picture

I have an idea...
because different fields don't always use "value" for array storage, you should flatten the array, kill empty values, and then do a count on it, to see if its greater then 0.

This
http://us3.php.net/manual/en/function.array-values.php#86784

With this

//kill empty values in the 1D flat array
foreach ($array as $key => $value) {
    if (is_null($value) || $value=="") {
        unset($array [$key]);
    }
} 

And finally this
http://us3.php.net/count

or is the problem more complicated?

verikami’s picture

ok :-) but let's improve this particular function

here is my investigation:

for point (a) - case 'php code':

value: $element['group'][$fields]['field']['items']['0']['#item']['value']

should be rendered as php code, but I don't know the function...

for point (b) - case 'all fields':

first - to have all fields, not only text:

 //: this line

$sql = db_query("SELECT field_name FROM {content_node_field_instance} WHERE type_name = %d AND widget_module = 'text'", $element['group']);

//: should be

$sql = db_query("SELECT field_name FROM {content_node_field_instance} WHERE type_name = %d", $element['group']);

//: 

and then:

 //: this statement

if (!empty($element['group'][$fields]['field']['items']['0']['#item']['value'])) {
  $node->content['fieldgroup_tabs'][$group_name] = $element;
  unset($node->content[$group_name]);
}

//: should be

if (
  !empty($element['group'][$fields]['field']['items']['0']['#item']['value']) || //: 4 text
  !empty($element['group'][$fields]['field']['items']['0']['#item']['nid']) || //: 4 node reference
  !empty($element['group'][$fields]['field']['items']['0']['#item']['uid']) //: 4 user reference
  ) {
  $node->content['fieldgroup_tabs'][$group_name] = $element;
  unset($node->content[$group_name]);
}

//: 

...but I don't know other possible cases and this is only a sketch...

...and this not works:

 //: why ???

$a = $element['group'][$fields]['field']['items']['0']['#item'];

foreach ($a as $key => $val) {
  if (!is_null($val)) {
    $node->content['fieldgroup_tabs'][$group_name] = $element;
    unset($node->content[$group_name]);
  }
}

//: 

v.

mikeytown2’s picture

I know computed field just evals the code

/**
 * Theme function for 'default' text field formatter.
 */
function theme_computed_field_formatter_default($element) {
  $field = content_fields($element['#field_name']);
  // For "some" backwards compatibility
  $node_field_item['value'] = $element['#item']['value'];
  eval($field['display_format']);
  return $display;
}

so with that you get the $display variable, which is '' if its empty

Imagefield is the other tricky one and that doesn't look too bad

function theme_imagefield_formatter_image_plain($element) {
  // Inside a View this function may be called with null data.  In that case,
  // just return.
  if (empty($element['#item'])) {
    return '';
  }

  $field = content_fields($element['#field_name']);
  $item = $element['#item'];

  if (empty($item['fid']) && $field['use_default_image']) $item = $field['default_image'];
  if (empty($item['filepath']))  $item = array_merge($item, field_file_load($item['fid']));

  $class = 'imagefield imagefield-'. $field['field_name'];
  return  theme('imagefield_image', $item, $item['alt'], $item['title'], array('class' => $class));
}

returns '' as well.

What does nodereference use to render?

verikami’s picture

thx. for this inspiration :-) ...I have to think a while now....

...and nodereference simply displays related nodes - I like this feature very much :-)

you could see it in action @ 'my' site (Gender Studies at University of Warsaw, so mostly polish lang) with group pages (eg.: http://www.gender.uw.edu.pl/en/group/jacek.kochanowski)

v.

robbertnl’s picture

Already a new patch available with the new suggestions?

verikami’s picture

not yet... another issue is where view is referenced, so another: if (!empty...)...?
it doesn't look very nice...

...and there is also problem with permissions... it is too difficult - could SOMEBODY help...?

heacu’s picture

subscribing

stoinov’s picture

subscribing

nedjo’s picture

Thanks for the posts. This is a complex issue. Probably our best bet is to follow CCK as closely as possible. How does it determine whether or not to render a field?

robbertnl’s picture

I don't use views in my tabs, so i am wondering if someone have a patch without the (currently not supported) views-reference functionality?

tlogan’s picture

I have applied the patch from #1 to my site and it fixes the problem for me (for now anyway). Will this be rolled into the module at some point?

tlogan’s picture

Well, that didn't last long. It now does not meet my needs. :-)

I have some fields that have permissions set. User1 can only view and edit fields on Tab1. User2 can view/edit fields on all tabs. It works fine until User2 fills in data on Tab2. Then User1 sees Tab2 even though there are no fields visible to him on that tab.

I changed the code to this and it seems to work now...

  //Patch starts
  for($i=1;$i<99;$i++){
    $sql = db_query("SELECT field_name FROM {content_node_field_instance} WHERE type_name = %d ", $element['group']);
    while($field = db_fetch_array($sql)){
      foreach($field as $fields){
        $myaccess = user_access('view '.$element['group'][$fields]['field']['items']['0']['#field_name']) ;
        if(!empty($myaccess)){
          $node->content['fieldgroup_tabs'][$group_name] = $element;
          unset($node->content[$group_name]);
        }//if
      } //foreach
    } //while
  }//for
  //Patch ends
robbertnl’s picture

StatusFileSize
new1.66 KB

The original patch doesn't work for me, since i am using node references as well. Suggestion #3 (by veriKami) seems to work for me. So i am using it now and made a new patch of it.

robbertnl’s picture

StatusFileSize
new1.77 KB

I added support for view-references (as suggested by #7).

I don't understand this line in the original patch:
//Patch starts
//for($i=1;$i<99;$i++){

Why? It doesn't make sense to me, making a db connection 99 times.
Can someone explain this?

nedjo’s picture

Status: Active » Needs work

Thanks all for your work on this issue.

It's clearly a priority to address. However, I don't think we've hit yet on the best approach. The patch as it stands will need to be edited for each new type of field. I might possibly apply something like this as a short-term hack. But I'd prefer to see a solution that doesn't need this hacking.

I think what we need to try to understand is: how does CCK avoid rendering fieldsets unnecessarily? Then we can try to follow the same logic.

verikami’s picture

I totally agree with nedjo - it is not the best approach for 'real' patch and good code - but if you look @ content.module... well - 4 me (4 now) it's too advanced level... and too many lines...

& @#15 (robertini) - yes - why FOR...?...

and (in addition) as is reported here:

#320030: Can't see uploaded files - CCK arays for them are empty

the permission problem is (generally) another important point...

matt_c’s picture

Subscribe

prom3theus’s picture

Hello, I'm just read this issue, and I think, the solition for what nedjo said is not in the content.module, but in the cck/modules/fieldgroup/fieldgroup.tpl.php
In that tpl, that's only one if...endif statement. I think it's a starting point for a trace back on this module's theme hook... so I think the solution for the problem is in the fieldgroup.module at theme_fieldgroup_fieldset() function.

robbertnl’s picture

StatusFileSize
new18.65 KB

It is still not the best solution but i changed the patch a little more. Now you don;t get any error if there are no tabs left (if all tabs are empty)

greg.harvey’s picture

Could you re-roll this patch so it doesn't try to replace the entired module? It should only contain the altered code and a few lines of context, or it's too hard to review. (And it doesn't apply.) =(

greg.harvey’s picture

Ps - #19 makes an excellent point. What has happened to '#children' in the fieldgroup object in this module? It simply isn't available for some reason, but is in the fieldset module itself. Anyone know how/where that is set and why it is not set in cck_fieldset_tabs?

Therein lies the key, IMHO. =)

greg.harvey’s picture

Actually re-rolled the patch in #20 myself after reviewing the previous version and seeing where the code changed. Like others have said, this is not a final solution but it does work. Nice one. =)

I had a brief look in to where $element['#children'] gets set - but I think it's to do with CCK hooks, etc. - so I think it needs someone who is already familiar with module building for CCK to tackle this in a more elegant way. If we can get the same $element['#children'] to appear in this module (it's access from a template_preprocess in fieldgroup) then we can solve this easily.

nigelcunningham’s picture

Subscribing.

apmac’s picture

Subscribing.