[development] Using array_merge_recursive() in _system_theme_data()

[Larry Garfield]

Hi Steve.

The problem with that approach is that we do not, in fact, want scripts.js to 
remain there in the child theme's info file.  If it is, then later processing 
will read that as the child theme wanting to declare scripts.js, and if there 
is a parent theme that also declares scripts.js then it will get removed, 
because the child theme is declaring scripts.js.  

That is, using array_merge_recursive() means that every theme will always 
declare scripts.js, even if it doesn't want to, and therefore scripts.js in a 
parent theme will always be overridden, even if you don't want it to be.  Ibid 
for stylesheets, etc.  For block regions and features, for instance, it would 
be even worse as there's no guarantee that a child theme will even support a 
region its parent declares, in which case you can have users trying to assign 
a block to a region that exists only in the admin, not in the theme.  Bad 
stuff. :-)

The declaring of any default for styles and scripts is itself the bug that 
needs to be fixed.  We didn't realize until after feature freeze that a side 
effect of that behavior is that a theme that declares no scripts, for instance, 
will therefore implicitly define scripts.js, which removes the scripts.js from 
the parent theme.  Hilarity ensues.

Instead if the default is to declare nothing, everything works as expected at 
the cost of themes needing to be explicit about all of their styles and 
scripts.  I'd say that's a preferred behavior anyway.

Also, normally an implementation detail issue like this should be discussed in 
a relevant issue queue, rather than the dev list, so that it's properly 
publicly archived and searchable.  

Merry Christmas!

On Wednesday 24 December 2008 11:15:54 pm Steve Edwards wrote:
> While working with a couple jQuery files in a recent D6 project by adding a
> second script to the theme.info file, I came across the issue where when a
> script is added in theme.info, it wipes out the default reference to
> script.js, so you also have to add script.js in theme.info so it is
> available.  I was discussing this with Crell in #drupal, and it was
> suggested to me that I write a patch to fix this.  So I figured I'd take a
> shot at my first core patch and tackle it.
>
> After spending a few hours going through _system_theme_data() and
> drupal_parse_info_file() with my handy dandy Komodo debugger, I've finally
> figured out the problem.  It lies in this line in _system_theme_data():
>
> $themes[$key]->info = drupal_parse_info_file($theme->filename) + $defaults;
>
> In particular, it's how the array returned from drupal_parse_info_file() is
> merged with $defaults.  Because it's using the + operator, any duplicate
> keys are not overwritten (as specified at
> http://de3.php.net/manual/en/language.operators.array.php).  Let me
> illustrate by showing the two arrays that are to be merged (this was
> created by adding scripts[] = script2.js to pushbutton.info).
>
> //array returned from drupal_parse_info_file()
> $info = array(
>        'name' => 'Pushbutton',
>        'description' => 'Tabled, multi-column theme in blue and orange
> tones', 'version' => '7.x-dev',
>        'core' => '7.x',
>        'engine' => 'phptemplate',
>        'project' => 'drupal',
>        'datestamp' => '1230077295',
>        'scripts' => array('script2.js'),
>      );
>
> //defaults as defined in system_theme_default()
> $defaults = array(
>      'regions' => array(
>        'left' => 'Left sidebar',
>        'right' => 'Right sidebar',
>        'content' => 'Content',
>        'header' => 'Header',
>        'footer' => 'Footer',
>      ),
>      'description' => '',
>      'features' => array(
>        'comment_user_picture',
>        'favicon',
>        'mission',
>        'logo',
>        'name',
>        'node_user_picture',
>        'search',
>        'slogan',
>        'main_menu',
>        'secondary_menu',
>      ),
>      'stylesheets' => array(
>        'all' => array('style.css')
>      ),
>      'scripts' => array('script.js'),
>      'screenshot' => 'screenshot.png',
>      'php' => DRUPAL_MINIMUM_PHP,
>    );
>
> The problem is that both have a [scripts] key, so since duplicates aren't
> overwritten, only one script array remains.  Which one remains depends on
> the order of the arrays in the statement (i.e.
> drupal_parse_info_file($theme->filename) + $defaults vs. $defaults +
> drupal_parse_info_file($theme->filename)).
>
> The way to fix this as best I could tell is to simply use
> array_merge_recursive() to merge the arrays:
>
> $themes[$key]->info =
> array_merge_recursive(drupal_parse_info_file($theme->filename), $defaults);
>
> What this does is merge the [scripts] arrays together so that instead of
> only getting one as the result, both are added:
>
> [scripts] => Array
>      (
>          [1] => script2.js
>          [2] => script.js
>      )
>
> This is also true for the regions, features, and stylesheets arrays; you
> have to replace everything in theme.info if you add more items there, and
> using array_merge_recursive() fixes that problem.  However, problems do
> arise for the non-array default items, such as description and screenshot. 
> Since there is one in each array, the value at [description] and
> [screenshot] is an array instead of a string, so t() throws a "Warning:
> Illegal offset type in isset or empty in t() (line 1057 of C:\Program
> Files\Apache Software
> Foundation\Apache2.2\htdocs\drupal7\includes\common.inc)." error, and no
> screenshot is displayed on the themes page.
>
> So, in my opinion, it seems the best thing to do is:
> 1) Use array_merge_recursive() instead of + to merge the arrays;
> 2) Remove 'description' and 'screenshot' from the default settings in
> system_theme_default()
>
> I suppose code could be added to handle the description and screenshot
> values so only one is used, but what I've suggested seems neater.
>
> Sorry for the book length email, but I've never been accused of leaving out
> details.:-)  Since this is my first core patch attempt, I just wanted to
> check with other more experienced core developers to see if there's
> something I'm not aware of that I need to be aware of.
>
> Thanks.
>
> Steve

-- 
Larry Garfield
larry at garfieldtech.com