Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Subtheme generation requires kalatheme to be default theme #215

Open
soniktrooth opened this issue Oct 30, 2015 · 8 comments
Open

Subtheme generation requires kalatheme to be default theme #215

soniktrooth opened this issue Oct 30, 2015 · 8 comments
Labels
Milestone

Comments

@soniktrooth
Copy link
Contributor

When trying to generate a sub theme nothing happens unless kalatheme is selected as the admin theme. I have tracked it down to the following code inside of kalatheme_prepare_config_form() :

  if (isset($form['#submit']) && is_array($form['#submit']) && !in_array('kalatheme_custom_bootstrap_library_submit', $form['#submit'])) {
    // Add our submit function first
    array_unshift($form['#submit'], 'kalatheme_custom_bootstrap_library_submit');

The 'Build and enable a custom subtheme' checkbox is ajaxified and checking it rebuilds the form.

  • When Kalatheme is default theme kalatheme_prepare_config_form is run twice, first time $form['#submit'] doesn't exists so we never enter the if condition. Second time it does exist so the submit handler is added.
  • When Kalatheme is NOT default theme kalatheme_prepare_config_form only runs once, $form['#submit'] doesn't exists so we never enter the if condition.

WHY the function gets run twice when kalaltheme is default and once when not is the big mystery here.

@soniktrooth soniktrooth added this to the 7.x-3.2 milestone Oct 30, 2015
@soniktrooth soniktrooth self-assigned this Oct 30, 2015
@soniktrooth
Copy link
Contributor Author

Ok, so I think the root of this problem is here: https://www.drupal.org/node/943212

system_theme_settings() manually calls theme specific settings form alters for all themes:

 // Call theme-specific settings.
      $function = $theme . '_form_system_theme_settings_alter';
      if (function_exists($function)) {
        $function($form, $form_state);
      }
    }

However if the currently enabled theme implements a $theme . '_form_system_theme_settings_alter'then it will get called again via the regular form_alter flow.

So the first time through this gets called by system_theme_settings() we haven't made it all the way through drupal_get_form() yet and so the form isn't built out yet in a way that is expected inside a form_alter (e.g. with all the '#this' and '#that') and so unexpected things start to happen and we can't rely on other code not over writing items in the array like our submit and validate handlers for example.

Armed with this knowledge I'm putting together a PR that I think would let us fix this.

@soniktrooth
Copy link
Contributor Author

@pirog—sorry to keep pinging you on these things but I wouldn't mind your eyes on the above PR to see if I'm overlooking something.

The if statement that I removed was very specific, however, I assuming that it was to suppress errors due to the fact that sometimes #submit didn't exist and we didn't want to add our callbacks twice because the function was running twice.

@pirog if you can confirm that there wasn't some other circumstance we were trying to avoid then this should be happy.

I've tested it locally with kalatheme as default and with seven as default. I guess it would probably be wise to test on Pantheon too seeing that is another place we support subtheme generation.

@pirog
Copy link
Contributor

pirog commented Oct 31, 2015

@soniktrooth

I think the reason we had this check was that sometimes kalatheme_custom_bootstrap_library_submit and kalatheme_custom_bootstrap_library_validate would end up being added to #submit/#validate twice.

Could dedupe the arrays further down with array_unique? This function is generally pretty costly at scale but i think on the order of magnitude of submit and validate arrays it probably is a NBD.

@soniktrooth
Copy link
Contributor Author

@pirog That's good because I think I got to the bottom of why those callbacks were being added twice. See my comment above for full details but basically system_theme_settings() calls $theme . '_form_system_theme_settings_alter' manually and it ends up getting called again by the normal form_alter flow when loading the theme settings form for the currently enabled theme. In light of all of that and your comment here I have adjusted my PR to make sure we're not getting the callbacks added twice. It's kinda complex and gross logic but I can't see a better way around it. I've tried to explain it all in the comments so hopefully this is fine.

@soniktrooth
Copy link
Contributor Author

@RobLoach was there a reason you added the above link?

@soniktrooth soniktrooth assigned RobLoach and unassigned soniktrooth Mar 16, 2016
@soniktrooth
Copy link
Contributor Author

#216 has been tested and merged

@wecodeitnl
Copy link

I've generated two subthemes, one as default and the second as an admin theme, but I'm not able to save the default theme settings. If I disable the admin theme everything works fine. Has this something to do with the issue above?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants