Following up on #761608: Missing theme settings values because list_themes() has inconsistent theme object data, I noticed that the 'sub_themes' key in the array returned by list_themes() is only set when the theme has subthemes; otherwise, it doesn't exist at all.

But the documentation sort of implies it would always be set (and be an empty array in the case of a theme without subthemes):

+ *   - 'sub_themes': An unordered array of sub-themes of this theme.

JohnAlbin pointed out that the same is true for many other keys in this array; they will be completely absent in the returned array if there is nothing to store in them.

We should document this behavior so that module authors know to check these keys with isset() before using them.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Issue tags: +Novice

Do you know which items in the array fall into this category and which don't? It's not immediately obvious from the code... I guess we would need to go through both system_list() and _system_rebuild_theme_info() to see which items in list_themes() are always set up, and which ones need to be checked with isset().

That could be a good project for a Drupal novice with good PHP skills... hopefully. :)

sukotto100’s picture

Assigned: Unassigned » sukotto100

Well, I am at least the former half of the above statement, so I will give it a try. Will work on this over this weekend, 6/30/2012

sukotto100’s picture

Ok, hopefully this will make sense:

Question 1: dpm(list_themes()) on a vanilla d8 install actually returns more than documented:

  • type (String, 5 characters ) theme | (Callback) theme();
  • owner (String, 50 characters ) core/themes/engines/phptemplate/phptemplate.engine
  • bootstrap (String, 1 characters ) 0
  • schema_version (String, 2 characters ) -1
  • weight (String, 1 characters ) 0
  • prefix (String, 11 characters ) phptemplate

Do these need to be added to the documentation and is this issue the place to do it?

Question 2: Regarding the actual issue, indeed there are several keys which do not exist. I think these would need to be added:
a. all related to subthemes:

  1. base_theme: doesn't exist in subtheme until base and sub exist
  2. base_themes: doesn't exist in subtheme until base and sub exits
  3. sub_themes: doesn't exist in base until subtheme exists
  4. scripts:
  5. observation: was not present.
    actions:added "scripts[all][] = js/sample.js" to .info file
    result: present after clearing cache; comes from .info file

  6. stylesheets:
  7. observation: present
    removed "stylesheets[all][] = css/layout.css" from .info
    result: missing; comes from .info file

If someone can confirm I am on the right track, I can create the patch. Sorry if I am being too tentative. First timer, here.

jhodgdon’s picture

RE #3 - thanks for the detective work!

Regarding Question 1: The list_themes() function is normally reading items from system_list(), which gets them from the {system} database table. That table stores modules, themes, and theme engines, and several of its fields are not relevant for themes (see system_schema()). So... I don't think we need to add type, bootstrap, schema_version, or weight to the documentation... I'm undecided about "owner" -- it's used to calculate more relevant information in system_list() [the parent theme], so I think we can also leave that out. That leaves "prefix" -- and yes I think we should add that in this patch.

Regarding Question 2: that looks like exactly the right things to do!

Thanks!

sukotto100’s picture

Yay! I'll work on it tonight!

sukotto100’s picture

Status: Active » Needs review
FileSize
1.65 KB

Here goes...

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good, thanks!

Your patch just needs "the" added in a couple of places, and a couple of other minor wording/grammar changes:
- "are defined in .info file" -> "are defined in the .info file" (stylesheets/scripts documentation).
- prefix: should be "Defined in the theme's..."
- base_theme: "If subtheme, the name of base theme" => "If this is a subtheme, the name of the base theme"
- sub_themes: "If theme has sub-themes" => "If the theme has sub-themes"

Also, it would be great if you could update the list formatting in this documentation block to our standards (remove the '' around the keys):
http://drupal.org/node/1354#lists

sukotto100’s picture

makes sense, will do.

David_Rothstein’s picture

Sorry for not looking at this sooner... To answer #1, I didn't know offhand which keys were affected (and indeed, it seems like the only way to tell is to read through all those functions).

I just took a look now and the patch looks good (except for the wording/grammatical issues above), but one question: Is it really true that 'prefix' will be unset if not defined in the .info file? It looks to me like system_list() and _system_rebuild_theme_data() both define 'prefix' unconditionally.

sukotto100’s picture

I will double check and update the patch tomorrow.

sukotto100’s picture

Status: Needs work » Needs review
FileSize
2.48 KB

I made the grammar changes noted above.
I also checked the entire block against the standards defined in http://drupal.org/node/1354#lists.
I verified that David is correct, if not definded the .info file, prefix is still present, my mistake.
Hopefully this is GTG, thanks for your help all.

Status: Needs review » Needs work

The last submitted patch, list_themes_documentation-1663606-11.patch, failed testing.

sukotto100’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
dcrocks’s picture

Of the values taken from the .info file the only ones guaranteed to be set are those documented as default(look line 2490 in system.module(7.x)). So even required fields such as 'name' may not be set. Of course that leads to immediate errors and the database record is not even built.

sukotto100’s picture

Hmm. So based on #14, does this require additional editing? It looks like that would refer to:
req but not guar: 'filename': The name of the .info file.
req but not guar: 'name': The machine name of the theme.
If so, I might need some help with phrasing.

dcrocks’s picture

Since failure to specify 'name' in the .info file immediately generates an error that prevents a record being generated in the db it is probably moot if the docs don't say the name variable might be empty. 'filename' is always constructed from the path to the .info file. And your description of the other variables is still accurate. So I don't think you need to make changes. I just wanted to point out the location in code where you can confirm whether a variable is or is net guaranteed to be set.

sukotto100’s picture

Actually that was very helpful. Slowly all coming together:)

jhodgdon’s picture

Status: Needs review » Needs work

There's one line that ... I'm not sure what it's supposed to say, but it's not OK as is:

+ *   - prefix: Defined in the theme's engine nameprefix.

And I couldn't figure out what this meant at all:

+ *   - base_themes: An ordered array of all the base themes. If the first item
+ *     is NULL, a base theme is missing for this theme. Not set if no base
+ *     themes are present.

Other than that, I think this looks good!

sukotto100’s picture

Thanks for the input. Is this descriptive enough?:
prefix: Displays the theme engine prefix.
I'm pretty unfamiliar with the theming engine intricacies; if you have any recommendations, I'm all ears.

As for part 2, the first sentence comes from here: http://drupal.org/node/761608#comment-6075178

+ *   - base_themes: An ordered array of all the base themes. If the first item
+ *     is NULL, a base theme is missing for this theme. 

This is mine:
Not set if no base themes are present.
Would "Array key is not set if NO base themes are present." clarify?

sukotto100’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Oh, I see. So on the base themes, what you want to get across is that if no theme anywhere on the file system has a base theme, then the array element is not present, but if some have base themes and some don't, the ones that don't have NULL in that entry... or is it array(NULL)? So, how about something like this:

- base_themes: An ordered array of the base themes for this theme. This element will be missing if there are no themes in the file system that use base themes. If some themes have base themes but this one does not, the value will be array(NULL).

[if the value is actually just NULL, then obviously put that in instead.] Do we need to do something similar for the sub-themes value too?

Regarding "prefix", I think it should probably say just "The base theme engine prefix" without the word "Displays", since this is just an array of values and it isn't doing any displaying. :)

jhodgdon’s picture

Status: Needs review » Needs work

Setting back to "needs work" so we remember we need a new patch.

sukotto100’s picture

Got it, thanks. Will work on it this eve.

sukotto100’s picture

Status: Needs work » Needs review
FileSize
2.66 KB

I did some more testing to get a better idea how sub_themes, base_theme, and base_themes work.

  • sub_theme exists if another theme calls it a base theme, and that's it. so when the a subtheme goes missing, the sub-subthemes below it aren't set in the base theme's array.
  • base_themes shows an ordered array of base themes, unless one is missing, then that missing one is NULL.
  • base theme is based on the .info file.

Let me know if it is clear. thanks for your patience.

jhodgdon’s picture

I'm still confused about base_theme, base_themes, and sub_themes... So let's say we have this example:

a)
alpha.info - base theme is beta
beta.info - base theme is gamma
gamma.info - no base theme
delta.info - no base theme

What will the return value be (base_theme, sub_theme, and base_themes elements) for each theme?

b) Now let's say that one of the themes alpha/beta/gamma is deleted, what will sub_theme etc. be for alpha, beta, and gamma?

If you can answer those questions, then I'll see if the documentation there makes sense and is correct. Thanks!

jhodgdon’s picture

Status: Needs review » Needs work

Forgot the status change... I'm pretty sure the documentation in the last patch doesn't tell me the answers to the questions in #25, and it should.

dcrocks’s picture

If I understand how the code works base_theme is the immediate ancestor of this theme if it has one, base_themes is an ordered(hierarchical?) list of all the parents of this theme(ie, parent's parents until a parent theme with no parent is found), which will be empty if base-theme is unset, and sub_themes is an unordered list of all the child themes of this theme(ie, this theme is listed in some theme's base_themes), which may also be empty. Or, sub_theme may be a list of all themes with base themes, but why have that list on a per-theme basis? I'm still trying to figure that out.
Since drupal doesn't flag an error if some theme specifies as base theme a theme that doesn't exist in the installation base_themes and sub_themes may have null elements.
Also, base_themes and sub_themes are dynamic variables and don't exist anywhere outside of memory while base_theme is maintained in the theme's $info entry in the database.

sukotto100’s picture

Ok, to restate #26, we are trying to document the return value of :

  • base_theme
  • sub_themes
  • base_themes

when the theme has a present or deleted base or sub-theme.

Using the example stated above:

theme base_theme sub_themes base_themes
alpha.info - base theme is beta string: beta not set base_themes (Array, 2 elements)
gamma (String, 10 characters ) Gamma name
beta (String, 9 characters ) Beta name
beta.info - base theme is gamma string: gamma sub_themes (Array, 1 element)
alpha (String, 10 characters ) Alpha name
base_themes (Array, 1 element)
gamma (String, 10 characters ) Gamma name
gamma.info - no base theme not set sub_themes (Array, 2 elements)
alpha (String, 10 characters ) Alpha name
beta (String, 9 characters ) Beta name
not Set
delta.info - no base theme not set not set not set

So we can surmise that a theme needs to have a base theme or sub theme for those keys to be set. Therefore lets take Delta out of the picture. When a base theme is missing, say Gamma:

theme base_theme sub_themes base_themes
alpha.info - base theme is beta string: beta not set base_themes (Array, 2 elements)
gamma (NULL)
beta (String, 9 characters ) Beta name
beta.info - base theme is gamma string: gamma not set
(when the base theme is gone, so is this)
base_themes (Array, 1 element)
gamma (NULL)

When the terminal subtheme (alpha) is missing:

theme base_theme sub_themes base_themes
beta.info - base theme is gamma string: gamma not set base_themes (Array, 1 element)
gamma (String, 10 characters ) Gamma name
gamma.info - no base theme not set sub_themes (Array, 1 element)
beta (String, 9 characters ) Beta name
not Set

When an internal node/subtheme (beta) is missing:

theme base_theme sub_themes base_themes
alpha.info - base theme is beta string: beta not set base_themes (Array, 1 element)
beta (NULL)
(so gamma isn't associated either)
gamma.info - no base theme not set not set
(no subthemes at all listed)
not Set

I am hoping this makes it clearer; I'll hold off for some feedback before I submit a patch again. Sorry for dragging this out. Also, basically everything dcrocks states in #27 is true. I can also confirm that none of the arrays are "a list of all base themes on the system" or "a list of all subthemes on the system." I read that in older documentation and took it a fact (or maybe misread it).

jhodgdon’s picture

Thanks for the testing! I think your logic is good. Let's just get the conclusions into the documentation. :)

sukotto100’s picture

Status: Needs work » Needs review
FileSize
3.1 KB
jhodgdon’s picture

Status: Needs review » Needs work

This is much clearer, thanks! I think a couple of things could be made just a little better:

a) In base_themes... What is meant by "a hierarchical associative array"? I have never heard of this term before... How about this:

base_themes: If this is a sub-theme, an associative array of the base-theme ancestors of this theme, starting with this theme's base theme, then the base theme's own base theme, etc. Each entry has an array key equal to the theme's machine name, and a value equal to the human-readable theme name; if a theme with matching machine name does not exist in the system, the value will instead be NULL (and since the system would not know whether that theme itself has a base theme, that will end the array of base themes). This is not set if the theme is not a sub-theme.

b) Let's use "sub-theme" instead of "subtheme" throughout the documentation text.

c) Since there are two things that are the "name" of the theme, let's consistently say "machine name" vs. "human-readable name" instead of "name" in the documentation text ("name" is sometimes being used for both of these types of names, and if we consistently say "human-readable name" we can replace awkward wording like "theme's name from the theme's .info file").

d) The wording in sub_themes says "An unordered array of themes which define this theme as a
+ * base theme in their .info files.", but according to your research in #28 (and some wording from later in that paragraph), it's actually an associative array of both themes that define this theme as a base theme, and 2nd-order base themes, 3rd-order base themes, etc. So how about something like this:

An associative array of themes on the system that are either direct sub-themes (that is, they declare this theme to be their base theme), direct sub-themes of sub-themes, etc. The keys are the themes' machine names, and the values are the themes' human-readable names. This element is not set if there are no themes on the system that declare this theme as their base theme.

e) I actually think we can get rid of the language about "if the themes are not defined" in the sub_themes element, given the suggested wording in (d). The reason is that if theme B theoretically says theme A is its base theme, but theme B is not present, then ... well, how would the system know about theme B in the first place to try to include it in theme A's sub_themes array?

sukotto100’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

Ok, that looks better. Thanks a ton for the help. LMK if this is not ok.

jhodgdon’s picture

Status: Needs review » Needs work

There are still a couple of places that say "name" without specifying machine vs. human-readable:

+ *   - engine: The name of the theme engine.
+ *   - base_theme: If this is a sub-theme, the name of the base theme defined

Other than that, looks good!

sukotto100’s picture

Status: Needs work » Needs review
FileSize
3.44 KB
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks great -- thanks for all your work on this! I'll get it committed shortly (the same patch will most likely work for both 8.x and 7.x).

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again -- committed to 8.x and 7.x. Great work!

sukotto100’s picture

my pleasure. thanks for your help:)

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