Replace theme functions with templates

I've been working on converting tables to use '#type' => 'table' but there is a core issue with '#access' => FALSE that affects node_grants in Search API. Created #3123459: Table element renders empty rows if the row #access is set to false.

I am going to work on converting theme functions templates before going into further improvements.

Connecting with #3023170: Test for compatibility with Drupal 9 as it has related discussion about this issue.

Here is the patch that converts theme functions to templates and aims to make as fewer disruptions as possible regarding the markup.

Any chance we can keep the functions in the .theme.inc file to keep this reviewable? At least as in intermediate step. It's not pretty and the downside is that we need to load it on every request, but for example field.module does a require_once __DIR__ . '/field.purge.inc';.

Addressed #6 and leaving preprocess functions in search_api.theme.inc for eaiser reviewing.

> and avoids increasing the size of the module file considerably.

For the record, that only benefits humans. Since we always load the extra file unconditionally, having it in a separate file actually makes it a tiny amount slower, but it's just one of hundreds of files being loaded on a typical requests ;)

I don't really care if we keep it or not, up to you.

Didn't review the documentation changes too closely, but looks like it's just fixing copy & paste errors.

#8 looks good. Thanks for cleaning that up, +1 on RTBC.

Regarding separation of the theme functions, I usually look for those at *.module file, but since theme functions eat already more than 50% of the current number of lines in search_api.module, I agree it might make sense to keep them in .theme.inc.

Answering #11: this might be an insane idea, but what if the preprocess functions we're adding in this patch would, before doing anything else, query the theme registry to see if the theme function has been overridden, and if so, throw a deprecation notice and then invoke that theme function directly to get the output? For example:

function template_preprocess_search_api_admin_fields_table(array &$variables, $hook) {
  $registry = \Drupal::service('theme.registry')->get();
  if (isset($registry[$hook]['function'])) {
    @trigger_error("The $hook theme function is deprecated, please override the template instead.", E_USER_DEPRECATED);
    $variables['table'] = call_user_func($registry[$hook]['function'], $variables);
    return;
  }

  // Do the normal preprocess stuff here...
}

Would this even work? Or am I misunderstanding the inspectability of the theme system? :) This might be total BS, but I thought I'd throw it out there.

From Slack: I'm not sure it's worth it to worry about BC there. I have a hard time coming up with a reason to override them. afaik, they just exist because you had no other choice in D7 to have a table with form elements, not to make it themeable. Why would anyone bother creating an admin theme to override these theme functions which are just about showing those backend UI's

So IMHO it's enough to do a change notice, mention it in the release notes and move on.

Works for me. Updating the issue summary to reflect this.

Change record added: https://www.drupal.org/node/3138575

I agree with #14 there is no need to worry about BC too much here. RTBC+1

Just FYI, after this change Acquia Search users can't access the search configuration anymore and they (we) need to downgrade/freeze the search_api version while waiting for Acquia to change the connector code accordingly:

Error: Call to undefined function theme_search_api_index() in acquia_search_theme_search_api_index() (line 618 of modules/contrib/acquia_connector/acquia_search/acquia_search.module).

So one could argue afterwards that a change notice wasn't quite enough. But maybe next time then.

Acquia has now issued a note in their knowledgebase: https://support.acquia.com/hc/en-us/articles/360048786374-Upgrading-Sear...