When you try to install the latest D7 HEAD via the command line (e.g., via the Drush wrapper or your own custom script) as of today you get this:

DatabaseConnectionNotDefinedException: The specified database connection is not defined: default in Database::openConnection() (line 1513 of /home/droth/web/drupal/includes/database/database.inc).

This appears to be an unintended consequence of #756762: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions. What happens is that the installer uses drupal_form_submit() to submit the database configuration form, and this now hits a call to drupal_theme_initialize() which tries to access the database before it's ready.

I believe the attached patch is a correct fix. The previous install code avoided initializing the maintenance theme for command-line installations based on the idea that it shouldn't be necessary, but that assumption is probably not correct. It ought to be harmless to initialize it regardless of whether we need it, and that fixes the bug.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Before anyone asks - yes, it is probably theoretically possible to write a test for this :) But it might not be easy: #630446: Allow SimpleTest to test the non-interactive installer

moshe weitzman’s picture

drupal_form_submit() always calls into the theme? That seems wrong. Or is this form special in some way? I have no problem with the proposed change but I'd like to understand what we've done.

sun’s picture

Title: Command line installations are broken » Programmatic form submissions don't need a theme
FileSize
2.65 KB

That's an unintended and unexpected change. Actually not introduced through the stated issue, but #757154: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter()

@moshe: We need to handle #theme suggestions somehow. In that issue, I already proposed that, if #theme is an array, and none of the suggestions is found, we need to fall back to element_children rendering. Fixing that would also fix this issue (as we'd no longer have to initialize the theme in the first place).

Attached patch is a stop-gap fix only.

sun’s picture

err. Of course, the opposite ;)

moshe weitzman’s picture

@sun - theme suggestions have to do with form rendering, not form submission. it is a bit dodgy that drupal_prepare_form() needs to initialize theme at all. it should be able to unconditionally set #theme_wrappers = array($form_name, 'form') and be done with it ... i'm not too familiar with this code though.

sun’s picture

ok, to clarify:

  • Right now (and since like ever), drupal_prepare_form() (previously drupal_build_form()) checks whether the built form specifies a #theme function.
  • theme_form() itself is a #theme_wrapper[s], not a #theme function; i.e., it gets the rendered children and wraps that into a FORM element.
  • Most forms do not specify #theme, so it's left to Form API to figure out whether there is a theme function for a particular form; based on the form_id.
  • In case no #theme was specified, we are checking the theme registry for whether there is a theme function that matches the form_id or the shared base form_id. In case one exists, we set it.
  • In case #theme was specified, we also check the theme registry, so as to make sure that the specified theme function suggestion actually exists. This is mainly caused by the fact that forms like node_form() or comment_form() wouldn't be able to suggest theme functions in the first place otherwise. I.e., those forms already know that they are TYPE_node_form, and so the theme suggestions for the form are array('TYPE_node_form', 'node_form'). Those suggestions should normally work as usual; i.e., theme() should use the first available. However, in the case of those two forms, none of the suggested theme functions exists in core, because Drupal core itself does not use them, so it would not make much sense to register + implement those theme functions, as that would mean we'd have to register + implement a whole flood of stub theme functions for lots of forms that are not actually used in Drupal core.
  • theme() itself handles suggestions pretty well already; trying each suggestion, and if all fail, returns nothing. It's just that we'd have to transfer that special theme suggestion handling into drupal_render()'s handling of #theme. I.e., it's possible that #theme is set, but an array of suggestions, and in case we invoked theme() with that array, but the result is still an empty string, then we need to render the element's children as usual.
  • Aforementioned behavior most probably works already. It's just that at some point, we added a watchdog() to theme() that alerts site admins of bogus/invalid theme attempts.

Hope this clarifies it a bit. Happy to clarify more, if needed. :)

moshe weitzman’s picture

Thats a good summary of this theme code which indeed has been there since 4.7. But I think all this themeing in drupal_prepare_form() is awkward. In D8 we should consider treating forms like everything else in the $page array. That is, if you want to theme a form you have to:

1. register a theme function/template
2. set #theme during hook_form_alter (this is new)

With that, we can rip all the themeing out of drupal_prepare_form()

David_Rothstein’s picture

Title: Programmatic form submissions don't need a theme » Command line installations are broken (programmatic form submissions don't need a theme?)
Status: Needs review » Needs work

Yup, the coupling between the form API and the theme system - via the call to drupal_theme_initialize() or equivalent in form.inc - has indeed been around a long, long time.... I'll trust Moshe on the dates :)

However, my original post is correct: #756762: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions is the commit that broke the command-line installation (I checked by rolling back the patch). It looks like the reason is that it moved that theme initialization code from drupal_build_form() to drupal_prepare_form(), thereby putting it in the path of programmatic form submissions.

I don't think the patch in #4 is correct, because the $form array gets passed all over the place (for example, to alter hooks), so we don't want $form to be different in the case of programmatic form submissions and non-programmatic ones... that could lead to unexpected results (e.g. if the alteration functions ever checked the value of #theme and acted based on it). Perhaps that is what @sun meant by a stop-gap fix only.

Anyway, if we could get that theme initialization code back into drupal_build_form() somehow that would be great, but I'm not sure we can do that easily or have to do that here -- it sounds like a non-critical followup. I think my original patch should be included at least as part of anything we commit here; the reason is that even if the form API winds up not initializing the theme system, some code that gets triggered somewhere else in the installer could easily do it. Always using the maintenance theme prevents that problem more generally.

sun’s picture

Technically, I'd just like to remove that watchdog() from theme() in case it doesn't find any of the specified suggestions. The watchdog() can be kept if the specified $hook is a string, i.e., when we're asked for a particular theme function and that function does not exist. But if theme() gets an array of suggestions, it should not bail out in the first place, since suggestions are just suggestions.

Doing that would allow us to remove the entire theme initialization from drupal_prepare_form(), and it would only apply automated suggestions in case #theme is not already set.

--

Aside from that, given David's reasoning in #8, I think that his patch is additionally a good idea to do -- although I fear it may slow down non-interactive installers.

sun’s picture

Status: Needs work » Needs review
FileSize
3.97 KB

This is what I mean.

sun’s picture

But obviously, I got the order of #theme suggestions in drupal_prepare_form() wrong.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Now thats what I'm talking about! Very nice untangling of theme and form APIs, and fixes the bug at hand.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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