hook_theme hurts developer experience big time. Now that we have the registry, can we drop it completely (or specialize it to corner cases)?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Priority: Normal » Critical

+1

I got the same idea by myself a few minutes ago.

Xano’s picture

Status: Active » Postponed (maintainer needs more info)

Okay, four 'wasted' hours later...

Since the registry already indexed all functions I used it to find out which functions were theme functions. Using the Reflection class in PHP5 I could find out the function parameters and their default values. So far so good. This would only require a small bit of code replacing the hook_theme() invokation.

But then came the real problem: hook_theme() doesn't only index theme functions, but also template files. It's possible list those files in the *.info files, but where should their default values be set? Right, in hook_theme().... or in preprocess functions. Since preprocess functions would then partly serve the same purpose as hook_theme(), I don't know if it's such idea to drop hook_theme() anymore. It could save maintainers time and efforts if their template files don't require default values, but if they do, maintainers wil end up with one preprocess function for each template file requiring default values.

We could at least simplify hook_theme() by forcing maintainers to use one name for both the theme function and the template file and to put template files in a 'theme' subdirectory of their module. Providing file names and paths in hook_theme() would not be necessary anymore this way. Also, people looking at the source files would always know where to look for template files, just like with translations.

Xano’s picture

I checked hook_theme() and I believe we can drop at least the file, patch, function and template arguments. I suggest hook_theme() provides one single name for each theme function/template. The registry can tell the theme layer if a theme function with this name exists. If it doesn't, use a template file with the same name located at /modules/module_name/theme/theme_function.tpl.php.

This approach saves us four arguments and makes sure template files are located at the same place for every module.

dvessel’s picture

The reason why hook_theme needs to register arguments is only for templates. Omitting them for theme functions would still work for normal functions. When Earl designed the theme registry, those arguments were there for situations where a theme converts them to templates.

If we can figure out how the preprocessor can sort out the arguments into the right keys, then there would be no need for the reflection class at all.

hook_theme() for node:

function node_theme() {
  return array(
    'node' => array(
      'arguments' => array('node' => NULL, 'teaser' => FALSE, 'page' => FALSE),
      'template' => 'node',
    ),
   ..

Each of those keys for arguments are converted into variables. $variables['node'], $variables['teaser'] and $variables['page'].

The problem is how do you get them keyed into the proper variables when it's not registered beforehand? Damien mentioned on IRC that we could wrap every theme template with theme functions to allow the reflection class to work but I'm not so sure about this. Seems like a step backwards.

Would this be done with the first template_preprocess_hook to sort it out?

function template_preprocess_node(&$variables) {
  $variables['node']   = isset($variables['arg_1']) ? $variables['arg_1'] : NULL;
  $variables['teaser'] = isset($variables['arg_2']) ? $variables['arg_2'] : FALSE;
  $variables['page']   = isset($variables['arg_3']) ? $variables['arg_3'] : FALSE;

  ...
}

With the change in theme() like so:

...
if (!empty($args)) {
  foreach ($args as $i => $value) {
    $i++; // Start at 1.
    $variables['arg_' . $i] = $value;
  }
}
...
}

It's crude but we wouldn't need to register any arguments.

Also, templates can be scanned with drupal_find_theme_templates. Having a strict directory structure I don't think is needed.

Xano’s picture

That's what I found out when I tried removing hook_theme(). There are just a few minor things that require the hook's existence.

Gurpartap Singh’s picture

up

Xano’s picture

Priority: Critical » Normal
Status: Postponed (maintainer needs more info) » Active

The render API is being used more and more. I have been thinking and AFAIK we can merge hook_theme() with hook_elements() for a unified rendering API. theme() would be replaced by drupal_render(), although theme functions themselves may remain. This allows for a better MVC approach and with hook_elements_alter() modules can hook into every part of the rendering process.

moshe weitzman’s picture

Title: Is it time to drop hook_theme? » Slimmer hook_theme()
Assigned: Unassigned » moshe weitzman
Category: task » bug
Status: Active » Needs review
FileSize
14.43 KB

Nice discussion.

Attached patch gets rid of file and path elements in favor of the code registry. I think this is a good start. Lots of cruft removed.

This should fix #314819: file includes for theming hooks fail with theme overrides. as well.

moshe weitzman’s picture

FileSize
13.92 KB

Actually, 'path' is used by templates in addition to functions. Thus it has to stay.

chx’s picture

'template' => 'maintenance-page',
- 'path' => 'includes',

'path' is used by templates in addition to functions. Thus it has to stay.

I believe this is an excess removal. If that gets fixed, I think this pretty piece is RTBC

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
13.9 KB

Only change is that line put back.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Xano’s picture

Issue tags: +Needs documentation

Yay! All we need to now is add some docs to Chronological.

moshe weitzman’s picture

Surprisingly, someone thought that this was done already; an entry for this already exists - http://drupal.org/node/224333#menu-file-path

moshe weitzman’s picture

Issue tags: -Needs documentation

fix tags

Pasqualle’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation
Pasqualle’s picture

Status: Needs work » Fixed

sorry the missing tag changes in comments confused me..

Pasqualle’s picture

Issue tags: -Needs documentation

remove tag

webchick’s picture

Priority: Normal » Critical
Status: Fixed » Needs work

FYI: This patch broke the installer. Try deleting settings.php and running install.php without one. When it goes to show the requirements page that tells you you need to copy the file, you get a notice and a big ol' nothing below the error message. It doesn't know how to find theme_status_report(). Testing bot doesn't fail because testing bot by design can't fail its install.

There are alternatives being explored at #464624: Allow limited functionality of drupal_function_exists in MAINTENANCE_MODE and #464556: Installer requirements check doesn't appear. Marking this issue needs work until this is resolved.

Damien Tournoud’s picture

I suggest rolling this back until we found a better solution.

JohnAlbin’s picture

subscribe

webchick’s picture

Category: bug » task
Priority: Critical » Normal

At #464556-13: Installer requirements check doesn't appear, Dries suggested rolling this patch back until we come up with a fix, so I've gone ahead and done that. Re-setting status.

We might want to merge back into this one the new hook at #464624: Allow limited functionality of drupal_function_exists in MAINTENANCE_MODE. Btw, this broke update.php as well (theme_update_page()).

chx’s picture

Rolling back now? When all was left of #464624: Allow limited functionality of drupal_function_exists in MAINTENANCE_MODE is documentation? Who do you think will work on that now? I am not for sure. I have sliced out precious time out of my workday to get that patch in and instead committing that and enhancing docs later we get this rolled back?

webchick’s picture

The patch rolled back with only minor fuzz. I don't see why that patch and this one couldn't be combined and we fix it all in one go.

In the meantime, people can actually install D7, which they haven't been able to do without asking questions in IRC for a week.

sun’s picture

chx pointed me here. I'll have a look this.

sun’s picture

Status: Needs work » Needs review
FileSize
18.46 KB

Tried a different approach, which chx did not seem to like (in IRC).

To put it in simplicity: We load all modules along with their include files that have been marked as required = TRUE in their .info files. I first considered to add another property, but then I thought - what's more than "required"? And, is running install.php/update.php even more or is it less than a usual Drupal request?

So, this works, but I need some feedback whether I should go back to chx's hook approach.

sun’s picture

Status: Needs review » Needs work

If that has to work with contrib, then we can only use the .info file approach, because we have no module list at install time (hence, no module_invoke_all()).

Possibly with a different (new) .info array though.

sun’s picture

Though I'm considering whether we should do

maintenance[] = system.admin.inc
maintenance[] = system.registry.inc

or simply

maintenance = TRUE

and load all includes (like in this patch) of a module that defines this.

Using the latter, #345118: Performance: Split .module files by moving hooks would need 2 special hunks less (the only manually added 2 hunks in that monster patch).

sun’s picture

FileSize
17.3 KB

Does it have to work with contrib?

If not, we could simply include system.admin.inc. Though I'm rather opposed to such hard-coded stuff. If it's only system module's files that are required, then attached patch works as well.

webchick’s picture

That was my question too. Why we couldn't just include the file explicitly before the call to the theme function.

I thought about contrib, but I don't believe there's any way for a contrib module to affect the installer until after the database is initialized.

sun’s picture

Status: Needs work » Needs review

So either we include explicitly, or we load system module including all includes, so whatever happens to functions in system.module, they'll be loaded and available (implementation of #29).

chx’s picture

Aye but the maintenance page is used for more than just install and update, see drupal_site_offline

sun’s picture

@chx: That is already taken care of, and, not changed in here:

  // Install and update pages are treated differently to prevent theming overrides.
  if (defined('MAINTENANCE_MODE') && (MAINTENANCE_MODE == 'install' || MAINTENANCE_MODE == 'update')) {
    // Load system.module with all include files.
    // drupal_get_path() depends on the database, which is not yet available
    // at install time.
    $path = dirname(drupal_get_filename('module', 'system'));
    $info = drupal_parse_info_file($path . '/system.info');
    foreach ($info['files'] as $file) {
      require_once $path . '/' . $file;
    }

    $theme = 'minnelli';
  }
  else {
    // Load module basics (needed for hook invokes).
    $module_list['system']['filename'] = 'modules/system/system.module';
    $module_list['filter']['filename'] = 'modules/filter/filter.module';
    module_list(TRUE, FALSE, $module_list);
    drupal_load('module', 'system');
    drupal_load('module', 'filter');

    $theme = variable_get('maintenance_theme', 'minnelli');
  }

All in else is for the regular maintenance mode/theme page. Not sure why we only load system and filter, but I don't care.

That said, if you look at the approach taken in #26, we could advance on that by loading all required modules in both cases, eliminating this special treatment and unify on a easily grokable standard. That might as well be a first step to a new DRUPAL_BOOTSTRAP_MAINTENANCE phase...

chx’s picture

Oh OK. Just load all required core modules and blocks, sure. I would provide a settings (UI less variable_get) to allow additional files just to have an out :)

sun’s picture

Assigned: moshe weitzman » sun
Status: Needs review » Needs work

Of course, we need load filter.module, because of filter_xss*() (sigh, again).

sun’s picture

sun’s picture

Status: Needs work » Needs review
FileSize
20.62 KB

Since chx asked for a variable, I reconsidered, and implemented a new .info file property, which, logically, is the same as a variable, only consistent:

maintenance = TRUE

That means, any module that wants to work at install/update time without having a database available, can use this directive.

Since #470632: Move filter_xss*() into common.inc is not yet committed, filter.info contains that directive as well (for now, can be removed afterwards).

sun’s picture

#332725: locale_inc_callback() is a thing of the past requires this patch, so we can load Locale module along with locale.inc in maintenance mode.

chx’s picture

Please benchmark this.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

Gábor Hojtsy’s picture

re @sun in #28: unfortunately this would not help solve #332725: locale_inc_callback() is a thing of the past because locale.inc is not at all in the locale module's location and is not referenceable through its info file. Ironically, for that to happen, we would need to fix #187398: Re-split locale module first, which was marked postponed on #332725: locale_inc_callback() is a thing of the past. So looks like our circle is back to fixing #187398: Re-split locale module first.

sun’s picture

Status: Needs work » Closed (won't fix)

Since #345118: Performance: Split .module files by moving hooks won't fix, we can revert the registry, and thereby this issue won't fix as well.

moshe weitzman’s picture

Status: Closed (won't fix) » Needs work

@sun - please don't hurt the queue when you are having a temper tantrum. With core as it stands today, this is hardly won't fix. We don't code to an imaginary core.

sun’s picture

Status: Needs work » Postponed

@moshe: This patch depends on the registry, but the registry's purpose and intended effect is "imaginary". If you want this issue to be fixed, then please go over there and express your support.

moshe weitzman’s picture

Status: Postponed » Needs work

Registry is in core. When it is rolled back, then this is won't fix.

chx’s picture

And people are surprised that core contributors "throw temper tantrum", see my comment about rolling this back. That happened a month ago, it should not have happened and now this issue is stuck and we are stuck with an inferior solution. Nice.

webchick’s picture

Got this bumped on my radar again.

The approach in #37 seems quite heavy-handed. It'll load, for example, .test files and .api.php files, unless I'm mistaken. The nice thing about chx's approach is that it will only explicitly load the code required, and it'll allow a contributed module to do the same if need be.

chx’s picture

FileSize
15.21 KB

I am such an idiot that even after taking a beating over it, I still rerolled it with the hook from the other issue. This probably is wrong from me because it encourages the 'does not matter what crap happens, chx wont be able to stand that an excellent patch gets stalled and work on it' behaviour but I am apparently a masochist.

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Priority: Normal » Critical

we can't ship without this fixed up, IMO

sun’s picture

Assigned: sun » Unassigned
sun’s picture

Status: Needs work » Closed (won't fix)

Looks like everything in this patch required the registry.