We are building a Drupal distribution with our subtheme of Corolla (subtheme of AdaptiveTheme). We wanted to enable and set default settings from the installation profile. Following Jeff Burnz suggestion at http://drupal.org/node/1578396, we are directly calling the submit function.

Is there a better way to achieve this ? Could we have an API function to save the settings without having to simulate a form submit ?

This is the code that we use:

  $default = '...'; // serialized settings

  $themes = list_themes();
  $my_theme = 'my_corolla';

  // Get the default values from the .info file.
  $my_corolla_settings = !empty($themes[$my_theme]->info['settings']) ? $themes[$my_theme]->info['settings'] : array();

  theme_enable(array('my_corolla'));
  variable_set('theme_default', $my_theme);

  $GLOBALS['theme_key'] = $my_theme;
  $config = array(
    'values' =>  array_merge(unserialize($default), $my_corolla_settings),
    'build_info' => array('args' => array($my_theme)),
  );

  require_once DRUPAL_ROOT . '/profiles/my_distribution/themes/adaptivetheme/at_core/inc/forms/at_core.submit.inc';
  require_once DRUPAL_ROOT . '/profiles/my_distribution/themes/adaptivetheme/at_core/inc/get.inc';
  at_core_settings_submit(array(), $config);

  variable_set('theme_my_corolla_settings', $config['values']);
  drupal_set_message(t('The configuration options have been saved.'));

  cache_clear_all();
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

Could we have an API function to save the settings without having to simulate a form submit?

My instincts always tell me yes this could be done, although I have discussed this with many people, some very experienced developers and no one has come with a solution, not even a bad one.

I reckon I can code my way around all the problems, I just need some time to think it through, and that's what I am woefully short on :(

What I mean is to hook into theme enable and set the settings at that point, rather than firing the submit.

nedjo’s picture

What I mean is to hook into theme enable and set the settings at that point, rather than firing the submit.

This would be easy enough to do in a module using

hook_themes_enabled()

. But there's not an obvious way to do this in a theme.

What is the minimum setup needed? It seems like the key detail is to generate the responsive CSS, without which the theme is not yet usable.

The best might be to isolate the minimum setup needed in a function that requires a minimum of input - ideally, none beyond the theme name - and then call that in the adaptivetheme submit handler.

Then any install profile or module could implement

hook_themes_enabled()

to call the function. Such an implementation might look vaguely like:

function example_themes_enabled($themes) {
  if (in_array('my_theme', $themes)) {
    $path = DRUPAL_ROOT . '/' . drupal_get_path('theme', 'adaptivetheme');
    require_once $path . '/inc/somefile.inc';
    at_core_generate_css_files('my_theme');
  }
}
ezra-g’s picture

How about having the subtheme implement hook_enable() and submit the form when the theme is enabled for the first time?

nedjo’s picture

@ezra-g: thanks for your input here. That would be great but I'm not finding a way for themes to respond when they're enabled. theme_enable() invokes a module hook but doesn't give anything for themes. This core issue is related: #1067408: Themes do not have an installation status. Form submission from a theme is also complicated as we can't use the regular methods like form_load_include() and drupal_form_include() which have assumptions that the forms they reference are provided by modules.

In any case the at_core_settings form handling is very complex and something we'd avoid wading into here if possible, since it seems like we need only a small part of what's triggered there: the generation of the CSS files.

It would be great if I'm missing something but if not it looks like we need a module or install profile in the mix.

ezra-g’s picture

Theme_enable calls the general hook_enable(), which, unless Im mistaken, themes can implement:

http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

Form submission from a theme is also complicated as we can't use the regular methods like form_load_include() and drupal_form_include() which have assumptions that the forms they reference are provided by modules.

I haven't yet looked into this in detail, but could we use drupal_get_form + drupal_form_submit inside inside of a hook_enable() implementation?

http://api.drupal.org/api/drupal/includes%21form.inc/function/drupal_for...

ezra-g’s picture

After further investigation, it seems like the challenge here is less about where to programatically submit the theme settings form (we could do it in an Installer task in the profile, for example) but how to submit it.

nedjo sketched some code in #2 - I had mixed results with various implementations with drupal_form_submit() and drupal_get_form().

ezra-g’s picture

After further investigation (and some pair programming with coltrane), we ended up with this snippet of code to troubleshoot getting the AdaptiveTheme and Commons_Origins settings to be set programmatically:

  module_load_include('inc', 'system', 'system.admin');
  foreach (array('adaptivetheme', 'commons_origins') as $theme_name) {
    $form_state = form_state_defaults();
    $form_state['build_info']['args'][0] = $theme_name;
    $form_state['values'] = array();
    drupal_form_submit('system_theme_settings', $form_state);
  }

Thanks to a debug_backtrace() at the top of at_core_settings_validate(), we discovered that the form fails validation the second time it's run with some undefined index errors. After further investigation we learned that the second time the form is submitted within a page request, most of the form's AdaptiveTheme settings weren't set in the $form and, as a result, in the $form_state.

These undefined indexes and lack of form values being set appear to be a result of the many require_once() calls that occur outside of defined functions, or that occur within functions but that include code that lacks function definitions.

As a result, it seems that they may only be getting included once per request, and lacking function calls/definitions, they are not executed for the second form.

Switching these require_once() calls to include() causes the function definitions that do exist to cause previously defined function errors.

As a workaround, installation profiles may be able to re-use the code snippet above without the initial foreach, running it once for AdaptiveTheme and once for the subtheme, each in an individual installer task so that the code is executed across two page requests.

Ultimately it appears that AdaptiveTheme needs to be refactored to move the various include files into functions that can be re-used.

ezra-g’s picture

An even more complete solution would abstract the submit handler logic to its own function that could be called without having to interact with the Form API. However, refactoring into functions would still be a huge improvement.

ezra-g’s picture

Title: Enabling a default theme in installation profile » Refactor require_once() instances into discrete functions
Category: feature » bug
Priority: Normal » Major
Issue tags: +commonslove

Re-titling with the action item here. Based on the issue here (Adaptive can't be successfully used as part of distributions), this seems major.

ezra-g’s picture

Status: Active » Needs review
FileSize
326.49 KB

Here's a first step towards this refactoring. The changes may appear extensive but really I've mostly just put the contents of .inc files in correspondingly named [name]_form(&$form) functions that take $form passed by reference.

With this patch applied, I can save the adaptivetheme and commons_origins settings form and get the same results (as far as I can tell -- page width and other style settings appear to be set) as before applying the patch.

I can also submit these forms programmatically via the code snippet in #7 without any validation or undefined index errors. The programmatic submission appears to successfully set the theme_[theme_name]_settings and theme_[theme_name]_files_directory variables. However, it appears that the page width isn't taking effect - I need to dig into AdaptiveTheme more to understand specifically why.

Diffing the theme_commons_origins_settings variable before and after shows only

-    [at-settings__active_tab] => 
+    [at-settings__active_tab] => edit-bigscreen

It would be great to get some review by folks who are more familiar with Adaptive's inner workings. If I can figure out how to set the page width, then Commons (and potentially other distributions) would be all set from my perspective.

ezra-g’s picture

Assigned: Unassigned » ezra-g
Status: Needs review » Needs work

This needs work so that all of the submit handlers run.

ezra-g’s picture

This update seems to get the submit handlers to run.

I'm able to successfully use the snippet from #2 in a custom callback on an already installed site, and the settings and CSS files appear to be generated correctly.

However, when run in an installer task, I get a slew of errors (once for each form submission) - It seems like this might be caused by responsive_panels_data_structure() assuming that it will always be run once the site theme has been set, or at least with global $theme_key set which I suspect may not be the case during the installer.

Warning: Invalid argument supplied for foreach() in responsive_panels_data_structure() (line 227 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Warning: Invalid argument supplied for foreach() in at_core_submit_reponsive() (line 40 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Notice: Undefined variable: smartphone_landscape_panel_settings in at_core_submit_reponsive() (line 162 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in at_core_submit_reponsive() (line 162 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Notice: Undefined variable: tablet_landscape_panel_settings in at_core_submit_reponsive() (line 289 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in at_core_submit_reponsive() (line 289 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Notice: Undefined variable: tablet_landscape_panels_data in at_core_submit_reponsive() (line 302 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Notice: Undefined variable: bigscreen_panel_settings in at_core_submit_reponsive() (line 356 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in at_core_submit_reponsive() (line 356 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).

Warning: Invalid argument supplied for foreach() in responsive_panels_data_structure() (line 227 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Warning: Invalid argument supplied for foreach() in at_core_submit_reponsive() (line 40 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Notice: Undefined variable: smartphone_landscape_panel_settings in at_core_submit_reponsive() (line 162 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in at_core_submit_reponsive() (line 162 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Notice: Undefined variable: tablet_landscape_panel_settings in at_core_submit_reponsive() (line 289 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in at_core_submit_reponsive() (line 289 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Notice: Undefined variable: tablet_landscape_panels_data in at_core_submit_reponsive() (line 302 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() (line 194 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/plugins.inc).
Notice: Undefined variable: bigscreen_panel_settings in at_core_submit_reponsive() (line 356 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core.submit.responsive.inc).
Warning: Invalid argument supplied for foreach() in at_core_submit_reponsive() (line 356 of /Users/ezra/Developer/htdocs/dev/acquia/commons_nightly/commons/nightly-3/profiles/commons/themes/contrib/adaptivetheme/at_core/inc/forms/at_core

ezra-g’s picture

Status: Needs work » Needs review
FileSize
331.27 KB

Here's a revised patch that, when responsive_panels_data_structure() is called and the active theme doesn't define its own Panels plugins, the function respons as if it had been invoked on behalf of at_core(), which I think is expected behavior.

With this patch, I'm able to successfully install Commons and have the Commons_Origins subtheme appear to inherit the expected settings in the install profile's hook_install() implementation:

/**
 * Implements hook_install().
 *
 * Perform actions to set up the site for this profile.
 *
 * @see system_install()
 */
function commons_install() {
  // Enable the Skye theme and set it as the default theme.
  theme_enable(array('adaptivetheme', 'commons_origins'));
  variable_set('theme_default', 'commons_origins');
  // Place main menu and site search blocks.
  $menu_block = array(
    'module' => 'system',
    'delta' => 'main-menu',
    'theme' => 'commons_origins',
    'visibility' => 0,
    'region' => 'menu_bar',
    'status' => 1,
    'pages' => '',
  );
  drupal_write_record('block', $menu_block);
  $search_block = array(
    'module' => 'search',
    'theme' => 'commons_origins',
    'visibility' => 0,
    'region' => 'header',
    'status' => 1,
    'pages' => '',
  );
  drupal_write_record('block', $search_block);
  // AdaptiveTheme requires that the system theme settings form
  // be submitted in order for its themes' settings to be properly set
  // and the resulting css files generated.
  module_load_include('inc', 'system', 'system.admin');
  foreach (array('adaptivetheme', 'commons_origins') as $theme_name) {
    $form_state = form_state_defaults();
    $form_state['build_info']['args'][0] = $theme_name;
    $form_state['values'] = array();
    drupal_form_submit('system_theme_settings', $form_state);
  }
}
bneil’s picture

ezra-g - our team has a sprint next week- we will provide a few sets of eyes towards this issue then if this isn't RTBC by then. Thank you for the work on this issue - it's been on our radar for some time but didn't really know where to begin.

Jeff Burnz’s picture

Its a very big patch :)

I'll try to grab some time to test this out later today/tomorrow, thanks for pinging me Ezra, been a bit flat out lately.

richardbporter’s picture

Not sure if you're looking for feedback but I tried using the patch and code from #13 and I'm still getting the PHP warnings on installation.

Invalid argument supplied for foreach() in responsive_page_layouts_data_structure() [warning]
(line 192 of
/adaptivetheme/at_core/inc/plugins.inc).

Also, it looks like some of the responsive CSS isn't being written to theme.responsive.layout.css after site install.

/* bigscreen three_col_grail */
@media only screen and (min-width:1025px) {
.container {width:100%}
-> missing all the css here
.at-panel .region {display:inline;float:left}.two-brick >...
}

Thanks.

Jeff Burnz’s picture

Category: bug » feature
Priority: Major » Normal

The patch seems quite incomplete, for example re-factoring settings forms into functions but then not calling those functions in theme-settings.php.

Jeff Burnz’s picture

Status: Needs review » Needs work
ezra-g’s picture

Thanks for the initial reviews.

Some clarification questions:

I'm still getting the PHP warnings on installation.

Could you post steps to reproduce? Do you mean enabling at_core on a fresh Drupal install? Or saving the theme settings form?

Also, it looks like some of the responsive CSS isn't being written to theme.responsive.layout.css after site install.

Could you describe how I could detect when the css is correctly written and what I should expect to see so that I can troubleshoot the patch?

The patch seems quite incomplete, for example re-factoring settings forms into functions but then not calling those functions in theme-settings.php.

Is there a specific function call that you see as missing? The patch in #13 includes several new calls to those functions and it's not clear to me which is missing.

I'll hangout in #drupal-adaptive - Feel free to find me to clarify. I'd love to re-roll this patch to address the points you raised. Thanks!

Jeff Burnz’s picture

So we have a record here - whats missing are all the functions for the various Extensions, so for example if I enable the Fonts extension the form will not show. Some have bad function names and result in PHP fatal errors (a couple of them are there, but are wrong).

First thing is to get those cleaned up - at least to the point of no errors and all forms loading.

Second, we need a pretty robust review of all settings/extensions to verify they are working, that we are passing in all the required parameters to the new functions, and that the data is being accurately/correctly returned.

I need some guidance on what has changed in the Plugin system and why. This is perhaps the most complex part of the theme and I need to be fully aware of what is going on here.

ezra-g’s picture

Status: Needs work » Needs review
FileSize
335.06 KB

First thing is to get those cleaned up - at least to the point of no errors and all forms loading.

After chatting in IRC, we clarified that some functions that alter the theme settings form by reference were in fact not being added. I believe I've fixed all of those in the attached patch.

Second, we need a pretty robust review of all settings/extensions to verify they are working, that we are passing in all the required parameters to the new functions, and that the data is being accurately/correctly returned.

I've tested to the extent I'm familiar with these extensions but it would be great to get more testing from someone who is more familiar with Adaptive and it's extensions. It seems like unit tests would be the ideal solution.

I need some guidance on what has changed in the Plugin system and why. This is perhaps the most complex part of the theme and I need to be fully aware of what is going on here.

There's no change to the business logic here - It's just been contained inside of discrete functions so that it can run consistently when called programmatically and more then once per page request. It only looks like an overhaul because the spacing changes cause most lines to be replaced.

Jeff Burnz’s picture

OK, i will test this out, regarding plugins.inc this is what I am asking about:

+      // If this theme doesn't define any Panels layouts, use those
+      // provided by at_core.
+      if (empty($data_structure)) {
+        $data_structure = at_load_plugins('adaptivetheme', $plugin_type = 'panels');
+      }
ezra-g’s picture

responsive_panels_data_structure() takes no arguments, and assumes that it's being called in relation to the side-wide current default theme (global $theme_key;).

However, sometimes we want to call this function on behalf of a theme that is not the site default or when that default hasn't yet been set (such as during installation). Without the patch, when this function is called on a theme that defines no new Panels layouts, it errors out.

ezra-g’s picture

Here's a re-roll that applies following the recent round of commits to AdaptiveTheme.

Given the scope of the issue here and the size of the patch, it would be great to get a review on this so that we can minimize additional re-rolling.

Jeff Burnz’s picture

First I would like to say I am sincerely apologetic about the recent updates and not having time to review this prior to this time. I am going to run the patch now and start testing in depth. I want to release a new version of AT asap, and this would be a great issue to nail before that happens.

ezra-g’s picture

Awesome - Thanks!

Jeff Burnz’s picture

Status: Needs review » Reviewed & tested by the community

I've spent about 4 hours testing and tweaking the patch and feel this is ready to commit. A few niggles but nothing major:

- a few undefined variables, nothing major and now fixed
- color module stuff needed a few extra parameters, now fixed
- two function names were inconsistent, now fixed

So, very few fixes required and a great patch. I think I have tested about as much of this as I possibly can at this stage, letting this into the wild might crop a few anomalies, so I think we commit this now and include it in the next version.

Note - at this stage I have not tested with Commons at all, I ran this on a standard Drupal install, I imagine it does not make any difference to Commons, if this patch was working before then it will work now, I have not altered the essential premise of the patch.

Jeff Burnz’s picture

Status: Reviewed & tested by the community » Fixed

Whoa! Committed this beast!

ezra-g’s picture

Status: Fixed » Needs work

Thanks for your help here, Jeff!

It seems I also owe an apology: I realized today that I overlooked a factor in testing my patches for this issue: As a stopgap measure for this issue, Commons shipped with some exported AT and AT subtheme settings: http://drupalcode.org/project/commons_misc.git/blob/refs/heads/7.x-3.x:/... .

When these exported settings are removed, the page layout is not properly saved in Origins when set the theme settings form is submitted programmatically.

For the time being, I've re-introduced this exported config back into Commons.

Jeff, I'd like to work with you to help troubleshoot the remaining issue either reading the theme's default settings, or saving those settings so we can close this issue once and for all.

Jeff Burnz’s picture

Status: Needs work » Fixed

I think that is a different issue, so lets close this as fixed and open another one. I am not sure how much time I have to be frank, I have much work to catch up on and since I do not get paid to work on these sorts of projects its very difficult for me to assign time.

Status: Fixed » Closed (fixed)

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

richardbporter’s picture

I have the same issue in #29. Has the new issue mentioned in number #30 been created?