Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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();
Comments
Comment #1
Jeff Burnz CreditAttribution: Jeff Burnz commentedMy 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.
Comment #2
nedjoThis would be easy enough to do in a module using
. 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
to call the function. Such an implementation might look vaguely like:
Comment #3
ezra-g CreditAttribution: ezra-g commentedHow about having the subtheme implement hook_enable() and submit the form when the theme is enabled for the first time?
Comment #4
nedjo@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()
anddrupal_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.
Comment #5
ezra-g CreditAttribution: ezra-g commentedTheme_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...
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...
Comment #6
ezra-g CreditAttribution: ezra-g commentedAfter 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().
Comment #7
ezra-g CreditAttribution: ezra-g commentedAfter 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:
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.
Comment #8
ezra-g CreditAttribution: ezra-g commentedAn 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.
Comment #9
ezra-g CreditAttribution: ezra-g commentedRe-titling with the action item here. Based on the issue here (Adaptive can't be successfully used as part of distributions), this seems major.
Comment #10
ezra-g CreditAttribution: ezra-g commentedHere'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
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.
Comment #11
ezra-g CreditAttribution: ezra-g commentedThis needs work so that all of the submit handlers run.
Comment #12
ezra-g CreditAttribution: ezra-g commentedThis 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.
Comment #13
ezra-g CreditAttribution: ezra-g commentedHere'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:
Comment #14
bneil CreditAttribution: bneil commentedezra-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.
Comment #15
Jeff Burnz CreditAttribution: Jeff Burnz commentedIts 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.
Comment #16
richardbporter CreditAttribution: richardbporter commentedNot 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.
Comment #17
Jeff Burnz CreditAttribution: Jeff Burnz commentedThe patch seems quite incomplete, for example re-factoring settings forms into functions but then not calling those functions in theme-settings.php.
Comment #18
Jeff Burnz CreditAttribution: Jeff Burnz commentedComment #19
ezra-g CreditAttribution: ezra-g commentedThanks for the initial reviews.
Some clarification questions:
Could you post steps to reproduce? Do you mean enabling at_core on a fresh Drupal install? Or saving the theme settings form?
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?
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!
Comment #20
Jeff Burnz CreditAttribution: Jeff Burnz commentedSo 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.
Comment #21
ezra-g CreditAttribution: ezra-g commentedAfter 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.
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.
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.
Comment #22
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, i will test this out, regarding plugins.inc this is what I am asking about:
Comment #23
ezra-g CreditAttribution: ezra-g commentedresponsive_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.
Comment #24
ezra-g CreditAttribution: ezra-g commentedHere'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.
Comment #25
Jeff Burnz CreditAttribution: Jeff Burnz commentedFirst 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.
Comment #26
ezra-g CreditAttribution: ezra-g commentedAwesome - Thanks!
Comment #27
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'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.
Comment #28
Jeff Burnz CreditAttribution: Jeff Burnz commentedWhoa! Committed this beast!
Comment #29
ezra-g CreditAttribution: ezra-g commentedThanks 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.
Comment #30
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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.
Comment #32
richardbporter CreditAttribution: richardbporter commentedI have the same issue in #29. Has the new issue mentioned in number #30 been created?