I have experienced significant friction from particularly theme_links() and theme_table() raising notices for items marked as optional and errors on some which could really be optional.

Putting all the inputs into an array rather than as individual parameters also gives less clear error messages - Undefined index: links in theme_links() (line 1417 of /opt/d7/includes/theme.inc). rather than Function theme_header requires the 'links' element in the $variables to be an array. in theme_links() (line 1416 of /opt/d7/includes/theme.inc).

So I am proposing two changes:
- Ternaries for the assignment of optional variables
- Presence and type checking for required values

This should keep the notices down and keep the error reports where they belong rather than errors being raised for Invalid input supplied for foreach ... at some point further down the call stack.

Attached patch implements changes for theme_links, I'll reroll across a wider front if the idea gets some support.

CommentFileSizeAuthor
#4 theme_etc.patch11.69 KBMichaelCole
#1 1001990.patch1.76 KBbellHead
theme_parameters_default_or_error.patch1.7 KBbellHead

Comments

bellHead’s picture

bellHead commented
StatusFileSize
new 1001990.patch1.76 KB

Rerolled patch that actually does what is claimed above :)

droplet’s picture

droplet commented
Category: feature » bug
Priority: Normal » Major

#1002380: theme_table Undefined indexes

require to fix all

MichaelCole’s picture

MichaelCole commented
Title: Defaults or errors on missing values in $variables for the theme_* family of functions » Defaults or errors on missing values in $variables for the theme_link family of functions

See http://drupal.org/node/1002380 for theme_table issue and patch.

MichaelCole’s picture

MichaelCole commented
Title: Defaults or errors on missing values in $variables for the theme_link family of functions » theme_* functions use associative arrays with optional indexes, but don't check if indexes are defined, causing Notice:'s
StatusFileSize
new theme_etc.patch11.69 KB

Hi, I didn't "get" it till I put together a patch, but bellHead is right.

The code comments indicate associative array arguments are optional, but if they aren't included, it throws a Notice: on the screen. This makes all arguments required, which isn't a good plan.

The attached patch covers all the theme functions but theme_html_tag.

They passed the theme tests on my install. This patch needs some feedback. Helper functions _isset_array and _isset_index can throw warnings (see bellHeads patch for the inspiration). I'm not 100% happy with the syntax of these functions, but it's the best I could come up with. Open to suggestions.

Too much?
Too little?
Just right?

Mike

MichaelCole’s picture

MichaelCole commented
Priority: Major » Critical
marcingy’s picture

marcingy commented
Priority: Critical » Major

This isn't critical.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Priority: Major » Normal
Status: Active » Needs work

It is a patch, though.

However, yikes. That's a huge order asking everyone who defines a theme_X() function to suddenly have to wrap any and all variables in these functions when we're two RCs in. I'm also not too hot on the approach; it introduces a weird Drupalism; no where else in PHP code do you need to use a function like _isset_index() and _isset_array(). Rather, you typically use a construct like:

$foo_variable = isset($bar_variable) ? $bar_variable : FALSE;

...this is standard PHP syntax.

So this seems like it needs work. I'm also not sure how PHP notices are "major". They're really "minor" problems that shouldn't be showing up at all if you're calling the functions properly, but I'll split the difference.

bellHead’s picture

bellHead commented
Title: theme_* functions use associative arrays with optional indexes, but don't check if indexes are defined, causing Notice:'s » theme_* functions use associative arrays with optional indexes, but use values without checking

The patch in #4 is probably a more correct procedural programming approach than mine in #1, but I do agree - Yikes!

Weird Drupalisms are certainly best avoided. This is mainly an attempt to bring the behaviour back in line after the Drupal code has done something odd though. By taking in an associative array the theme_* functions have avoided the use of the standard PHP features for supplying defaults for parameters and what little parameter type checking exists.

So in order to avoid the notices (and sometimes warnings or errors) resulting from using values without checking them something needs to be done. I think this is quite a big deal from a developer experience point of view. I'm not sure it's bug though as you can see from history.

The basic ternary statement solves the problem most of the time, as per webchick in #7

<?php
  $sad_panda = isset($panda['sad']) ? $panda['sad'] : FALSE;
?>

In order to deal with making expectations clear (as opposed to just doing nothing because expectations were not met which is a big DX nono) there needs to be a little something else. I think the least disruptive route to that end may be a function which wraps a default assignment and raising a warning.

<?php

........


    $sad_panda = isset($panda['sad'])
      ? $panda['sad']
      : drupal_default_and_warn(FALSE, 'Function patch_review wants to know if the panda is sad.');

.........

  }

  function drupal_default_and_warn($default_value, $warning_text) {
    trigger_error($warning_text, E_USER_WARNING);

    return $default_value;
  }
?>

A reasonable compromise?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

This problem is not specific to theme functions.

Almost all functions in Drupal expect that the correct arguments are put in and with the correct type. If not, they explode in more or less nice ways. This is imho bad and something that might need to be changed in Drupal 8 but I think it's really too late for this in Drupal 7. FYI, the opposite of what we are doing is called http://en.wikipedia.org/wiki/Defensive_programming.

Regarding theme functions, optional arguments obviously need some sort of existence check, simply to avoid E_NOTICE issues since that is a coding style rule.

However, if you don't pass in the required arguments, then that is by our current definition a bug in your code, tons of similiar issues (for example because of notices and errors within menu.inc) have been closed as won't fix/by design.

And if a required argument is not defined as such, then that is a documentation bug that needs to be fixed case by case (or all in one, but that could be a huge task). In your example, using theme_links() without links does make no sense to me and is documented as required according to http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_links/7 (required = not optional ;))

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Closed (works as designed)

There is nothing to do here. The addition of default arguments is done in theme().

You cannot call the theme_* functions directly. There are just not designed for that.

damien tournoud’s picture

damien tournoud commented

Also note that the theme system doesn't really have the concept of 'optional variable'. We could decide that variables which have a non-NULL default in the theme info registry are by essence "optional", but there is nothing that clearly state that anywhere. The definition of the variables structure is:

* - variables: (required if "render element" not present) An array of
* variables that this theme hook uses. This value allows the theme layer to
* properly utilize templates. Each array key represents the name of the
* variable and the value will be used as the default value if it is not
* given when theme() is called. Template implementations receive these
* arguments as variables in the template file. Function implementations
* are passed this array data in the $variables parameter.

Nothing that describes the optional vs. required status of the variables. All in all, this seems to say that all the variables are optional and should probably be clarified.

MichaelCole’s picture

MichaelCole commented
Status: Closed (works as designed) » Needs work

Re: Don't call functions directly, use theme()

If that's the case, then the API docs should say that. Otherwise, it half-works and is confusing.

I'm guessing we're not the only two people that will run into this issue.

How about a patch to update the docs? That seems appropriate.

Interested?

marcingy’s picture

marcingy commented
Status: Needs work » Closed (works as designed)

It is already documented here under the theme api http://drupal.org/node/722174

MichaelCole’s picture

MichaelCole commented
Status: Closed (works as designed) » Needs work

Ok, but it's not documented on the API page where I found the function.

http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_table/7

Is the documentation for people who already know the api, or for those who don't know how to use it yet?

marcingy’s picture

marcingy commented
Status: Needs work » Closed (won't fix)
Issue tags: -D7DX

Please read the document I refered too that tells you how to call theme functions, there is no reason to have a line every theme function stating don't call this directly as that is ineffect what you are asking for. The theme api page shows you how to call theme functions.

MichaelCole’s picture

MichaelCole commented

Yes, you already said the document describes how to call theme functions.

But the API doc in the link on api.drupal.org doesn't. That documentation doesn't describe how to use the function: "This is an internal function, which should be called through theme()".

API users shouldn't have to read the whole site to understand how to use one function. API's are external descriptions of how to "use" the function, not internal descriptions of how they "work".

https://secure.wikimedia.org/wikipedia/en/wiki/Application_programming_i...

An Application Programming Interface (API) is a particular set of rules and specifications that a software program can follow to access and make use of the services and resources provided by another particular software program that implements that API. It serves as an interface between different software programs and facilitates their interaction, similar to the way the user interface facilitates interaction between humans and computers.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, but there is only one way to add it to every theme_ function on that page.

By adding it to every theme function in the code. Not going to happen :)

Actually, there is even a link at the bottom of the page:

Related topics

Default theme implementations
Functions and templates that present output to the user, and can be implemented by themes.

That is linked and points to a page that exactly explains how theme functions are used. If a them function doesn't have that, then it's a bug.

droplet’s picture

droplet commented

all of theme function work in same way ??

http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_html...

theme_html_tag, it's not working well.

theme('html_tag');

even Ive given #tag a value, it still get an error.

Notice: Undefined index: #attributes in theme_html_tag()

but it's already defined default value in system_element_info(), right ?

marcingy’s picture

marcingy commented

@droplet see this comment from davidreid http://drupal.org/node/1004556#comment-3855112

droplet’s picture

droplet commented

@marcingy,

thanks.

so Im calling it from wrong way ?? If I call from theme function, then I should give all parameters?

why I trying to ask because I see few usages in core is in this way 

        $output .= theme('html_tag', array('element' => $js_element));

http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ge...

if it get rid of #attributes, then error occur.