Loosen the coupling between the user module and the installer/maintenance theme

Good idea! But variable used not only on maintenance theme so we need additional checks in template_preprocess()

seems only comment.module failed so it's easy to fix

Nice, makes sense to get this in before tackling #1153716: Decouple system module from the installer.

#1: decouple-installer-maintenance-theme-user-module-782672-1.patch queued for re-testing.

Re-rolled patch from #1 to apply cleanly.

This simple fix should make it pass. Not sure if is the right approach.

I'm currently trying to understand the internals of the installer, so that I can help the effort to rewrite it as a Symfony 2 app. Why do we load the user here?

What does user_access do in this case? It's unclear if this code accomplishes anything.

Can someone provide context to what going on?

I vastly simplified this, and also removed the static caching from user_preprocess(). The only part that would remotely benefit from the static cache is the call to user_access(), but user_access() is statically cached already.

So this actually revealed that the previously contained static caching hid a bug in testing.

The problem is that theme() registers the following preprocess callbacks:

template_preprocess()
template_preprocess_comment()
user_preprocess()

I.e.: Comment module is supposed to implement template_preprocess_comment(), but at the time it is invoked, user_preprocess() did not run yet, so $variables['user'] is always the anonymous user.

What we want is to run user_preprocess() before the template_preprocess_HOOK(). However, there are cases in which that would reasonably break existing implementations, since they rely on the other/existing order.

So what we need to do is to introduce a new preprocess phase for theme(), which allows modules to supply additional default variables for all templates, after template_preprocess() has been invoked; i.e.:

template_preprocess()
user_preprocess_defaults()
template_preprocess_comment()

Attached patch implements that.

I did not implement it as a hook directly in template_preprocess(), since I think that all of the template preprocessing should go through the theme registry system (which also gives extensions a chance to adjust/alter the callbacks when needed).

#16 passed all tests and resolves this issue. Anything not to like or can we move forward here?

Obviously, the required adjustment to the theme system was an unexpected surprise. However, that's really the only way to resolve this issue. In case the explanation in #16 is not sufficient, I'm happy to discuss and clarify further.

FWIW, this issue blocks #1549526: Change global $user into $session

#16: drupal8.install-user.16.patch queued for re-testing.

#17: Doesn't this add one full function call to every theme() call?

Couldn't we cache this instead and call from _default_variables and re-add the if ($user != $default_user) check?

Just a little concerned about performance. Besides that: Fine work :-).

Note that the additional preprocess function call is limited to templates (i.e., not theme functions).

The more interesting aspects from my perspective are really rather:

- Currently, only User module gets the chance to be involved in template default values by being hard-coded into the theme system.

- The theme system statically caches those default template variables, and even goes one step further and hard-codes the exact condition in which that static cache needs to be refreshed. Apparently, any change to $user is not taken up currently, unless the uid changes.

That entire hard-coded tight coupling doesn't sound right to me.

Unless I'm overlooking something big, this change boils down to a single additional function call per rendered template, whereas the function itself is very cheap. I doubt this is noticeable in any way.

Please also note that I've injected clone before the $user variable assignment. This prevents theme templates from futzing with the global $user.

Another idea from yourself ;-):

Could you not more easily do:

_template_preprocess_default_variables() {
// ...
  drupal_alter('template_default_variables', $variables);
}

/**
  * Implements hook_template_default_variables_alter().
  */
function user_template_default_variables_alter(&$variables) {
  global $user;

  $variables['user'] = clone $user;
  $variables['is_admin'] = user_access('access administration pages');
  $variables['logged_in'] = ($user->uid > 0);
}

And then when user is logged / logged out / whatever:

  // Clear the default values
  drupal_static_clear('template_preprocess');

Might want to change the drupal_static key in template_preprocess though to make more clear this is only for the default variables.

That way you use:

* module_implements() cache instead of theme registry
* You are not bitten by bad hacks.
* Use best practice to alter things
* It is cached properly.
* The cache can be reset easily.

The patch is much smaller and does not need to deal with the theme registry and add yet another function to the already complex mix.

Edit x-post with previous comment, but this solves all issues of previous comment.

Yes, I think that makes sense.

Let's see how it goes. :)

I double-checked which hooks are invoked in which occasions, and additionally, what Devel module is doing when switching users (since the $uid comparison for the static cache in template_preprocess() primarily targets Devel and Masquerade module functionality).

And a patch :-)

Note that I added an unset of password, sid and ssid properties as themers really should not accidentally dump or output the password of a user (even if its encrypted).

Incorporated the unset()s of user properties.

As sun wrote it independently from me, I feel it possible to RTBC this:

Does it solve the goal?

Yes, it removes two static hacks and allows user module to only add the $user vars when it is enabled.

Why does it work?

* Adding one alter call that is cached.
* Making sure the cached drupal_static data is cleaned on change of the cached data.

Why this way?

* hook_alter is the right choice as default data is altered.

Performance?

Neither better nor worse as the call to alter is in a static cached path.

DX?

* DX is improved as other modules / themes can now add variables to the (cached) default template variables.
* The $user object does not expose potentially dangerous settings (password) to the themer as they neither need this, nor should accidentally leave a {{ user.password }} in a template.

Full Win!

OK the _alter() hook + static + clearing doesn't feel great, but it's going to be used extremely rarely, especially for something where the static needs resetting. It's also a lot nicer for performance than implementhing hook_process() and having that run hundreds of times per request.

I don't think this interferes with the installer/kernel patch, there's plenty of this hard-coded stuff in the installer, that simply making it it's own Kernel is not going to have any effect on either way.

Will need a change notice for the new _alter() hook.

Also, can we get rid of this now?

core/authorize.php:drupal_load('module', 'user');

Thanks! Added http://drupal.org/node/1838470

authorize.php still needs to load user.module, since it calls into user_access().