I got 3 E_NOTICEs with the new version (2.5), I just jumped from 1.1 to 2.5... 8-)

It otherwise seems to work just fine.

So... one set of notices came from the foreach() over the $classes variable which may call collapse_text_id_safe() with an empty string. I suggest we don't call the function at all if $s is empty.

The other, I have a doubt about it...

  // the original is just: if ($tree['#title'] && ...) {
  if (!empty($tree['#title']) && ($tree['#collapse_text_default_title_flag'] == 'yes')) {
    $tree['#title'] = $default_title;
  }

Here you set the title to $default_title if it is defined. Is that really the intend?! Shouldn't you set the title if it isn't set?

Thank you.
Alexis

CommentFileSizeAuthor
collapse_text-6.x-2.x-notices.patch1.16 KBAlexisWilke

Comments

pukku’s picture

pukku commented

Hi! I have added a test for empty $s.

You said that you go three E_NOTICEs. Were they all from an empty $s?

Your second question is actually not a problem. The function that it is in is walking through the tree and updating the default title, if that is what was put there, to be the default title provided by the format or the user in a [collapse options] tag. Rather than send the options hash all the way down through the recursive functions, I am being lazy and using the out-of-the-box default title, and then replacing that in a second recursive call. I probably should just suck it up and send the $options hash through all of the recursion. But the code that you are looking at in particular is correct, because only proper FAPI elements have a #title attribute, so it's a short hand to make sure I'm looking at an actual FAPI array.

Ricky

AlexisWilke’s picture

AlexisWilke commented

Just raising the issue. 8-)

Now for entries that do not have the #title, it generates an E_NOTICE. The patch I attached shows the fix.

There is another E_NOTICE in that same function since the #collapse_text_default_title_flag may not be set either. Actually, a better simplification would be to unset it no matter what (I think the if() is not required.)

-  if ($tree['#collapse_text_default_title_flag']) {
+  if (isset($tree['#collapse_text_default_title_flag'])) {
     unset($tree['#collapse_text_default_title_flag']);
   }

equivalent to:

   unset($tree['#collapse_text_default_title_flag']);

I think. You should get E_NOTICEs too and thus test for those issues. 8-)

Thank you.
Alexis

pukku’s picture

pukku commented

Hi! I decided to fix the actual problem (which was my laziness) rather than the post-hoc fix I was attempting to do. I've modified the code to make the options array available through the entire recursive stack. This way, I can set the title at the right place.

I have one more fix that running with E_NOTICE made visible, and I will then release it...

Ricky

pukku’s picture

pukku commented
Assigned: AlexisWilke » pukku
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.