Problem/Motivation

Part of #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent

Convert all template_preprocess

Steps to reproduce

Proposed resolution

* Move all template_preprocess functions in update module to ViewsThemeHooks, including hook_theme itself
* Deprecate the old functions and move them to views.module, so that the file definitions can be removed
* Update all references, including direct calls in tests and @see/@covers in tests and templates.
* Figure out how to define initial preprocess callbacks based on the plugin definition and theme name.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3535680

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

berdir created an issue. See original summary.

berdir’s picture

Status: Active » Needs review

This might not be as hard as I feared, but it does depend on this central piece of magic:

       $initial_preprocess_class = 'Drupal\\' . $def['provider'] . '\\Hook\\' . Container::camelize($def['provider']) . 'ThemeHooks';
        $initial_preprocess_method = 'preprocess' . Container::camelize($def['theme']);

        if (method_exists($initial_preprocess_class, $initial_preprocess_method)) {
          $hooks[$def['theme']]['initial preprocess'] = $initial_preprocess_class . ':' . $initial_preprocess_method;
        }

I think we generally want less magic like this (service providers), but it's not enforced, just a default, and it feels in line with the kind of magic that this hook does.

There is also the bit about theme_file that plugins can in theory provide, but the new attribute classes are actually missing that and I want to deprecate the underlying 'file' key as well, so I just went ahead and added that deprecation right now. There are a bunch of them in contrib, but again, they would need to resort to a plugin definition alter hook for this in D12 with attributes anyway. And all they need to do for D12 compatibility is move the functions into their .module file (or convert to initial preprocess, but that would require 11.3 unless we backport this)

lendude’s picture

Left a small comment on de MR. Might just be me reading the flow wrong.

Yeah the code in #3 pretty magic, but it's not too bad I feel. It does need some figuring out what's going on, so if others agree this is the best way to go, it would benefit from a comment or being moved to a private method to document what is going on I think

nicxvan’s picture

@lendude does @berdir's response cover it for you?

lendude’s picture

Yeah that clears up my question! Thanks.

nicxvan’s picture

I'll do a full check when I get a moment

nicxvan’s picture

Status: Needs review » Needs work

Ok I did my review too:

No double _
All deprecations for the functions are __FUNCTION__
Versions are all correct
The functions call the correct method

I haven't done the zebra check yet but that probably won't work on this one.

Two comments on the MR

And the change records mentioned in slack are needed.

nicxvan’s picture

Status: Needs work » Reviewed & tested by the community

MR comments have been addressed!
Per 8 and a double check I think this is ready.

Change record also looks good!

nicxvan’s picture

I reviewed the new cr, looks good, the new commit was just to update the cr link

berdir’s picture

I now also created the second CR about theme_file (which, as mentioned in the CR, is basically already deprecated in D12 as we forgot to add it to the attribute classes and rarely used, so I thought it's OK to explicitly deprecate for D12).

FWIW, I'm not entirely happy about the approach that I implemented, writing the CR and explaining it reminded me of that again. Too much magic. An alternative is putting it on on the specific plugin class, that would be a much better location, but I don't really see a way to make that work with DI, it would need to be a static method. The ThemeHooks class approach has the advantage that DI works. But the whole feature is pretty magical already and entirely optional. You can alter it, you can also not define a theme key and fully define it yourself.

catch’s picture

Status: Reviewed & tested by the community » Needs work

A few merge conflicts in this one unfortunately.

berdir’s picture

Status: Needs work » Reviewed & tested by the community

Rebased, only had two conflicts on test classes, some use and docblocks in core/modules/views/tests/src/Kernel/ViewsPreprocessTest.php and core/modules/views/tests/src/Kernel/Handler/ArgumentSummaryTest.php. Should be fine to set back to RTBC assuming that test are green.

  • catch committed a448bcf2 on 11.x
    Issue #3535680 by berdir, lendude, nicxvan: Convert template_preprocess...

catch’s picture

Status: Reviewed & tested by the community » Fixed

I wonder a bit if there's a way to deprecate the magic theme stuff altogether, but this isn't the issue for that and I'm not sure exactly how we'd do it at all (except purely via change record?).

Committed/pushed to 11.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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