Convert theme engines into services

Yes, I think this sounds like a good idea, but I'm going to postpone this for now, since we have bigger fish to fry at the moment. :)

moving to the active twig sandbox.

Better title.

Marked #304486: Theme Engines as Modules as duplicate of this issue.

A lot of open questions:

• There is still $theme_engine. Should this keep to be the name or the actual instance of the theme engine.
• Theme engines can participate in preprocess functions. How do we represent this in the registry cache

Plugins? Really? There's no UI, and only one active theme at a time. This is a service, possibly with a negotiator class like Breadcrumbs and the theme itself have.

@Crell: (1) A theme can have parent themes, so there can be more than one theme at a time. (2) The concept and support for alternative theme engines still exists and has not been removed from core. (3) A sub-theme can use a different theme engine than its parent theme.

Whether plugins are the right answer, or whether a theme engine can just be exposed as a service, needs investigation.

Theme engines do not have an installation status and are not installed/enabled right now — all available theme engine extensions in the filesystem are automatically discovered when ThemeHandler::rebuildThemeData() rebuilds the information of available themes.

This means that a theme engine is available and usable by its mere existence. There is no owner/provider for a theme engine; i.e., no one registers a theme engine on behalf of it.

The only exception to that is CoreServiceProvider::registerTwig(), which apparently presents an architectural problem, since no other theme engine is able to do the same, equally early in a kernel boot, unless the theme engine would be backed by an additional module.

A theme engine can apparently have a good amount of configuration, as exemplified by the registration of Twig in CoreServiceProvider... (all in settings right now)

I hope these data points help us to find the right answer.

I'd say all the criteria sun just laid out lean much more heavily toward plugins than a single service.

$engine = \Drupal::service('plugin.manager.theme.engine')->createInstance($theme['engine']);

Not:

$engine = \Drupal::service('theme.engine');

Eclipse

For systems in which both plugins and services kinda work we do have an interesting correlation at the moment: Services choose themselves whether they should take part, plugins are chosen externally,
like in a config file/entity. Theme engines are chosen in the info.yml file.

Continued the work and got the installer working with it.

Yeah, it is a little bit trigger to figure it out with theme engines also being extensions.

Closed #1804864: Make theme engines classes as a duplicate.

I'm still not convinced of plugins here. We can register multiple theme engines as services; there's virtually no overhead to doing so. A given theme can specify one that it needs, and then we load that as appropriate... done. There's no UI configuration here. There is for what theme to use, but the theme->engine relationship is hard-coded into the theme at authoring time.

AFAIK we don't support a parent theme with a different engine; and if we do, I'd be happy to lose that ability if it simplifies the code.

AFAIK we don't support a parent theme with a different engine; and if we do, I'd be happy to lose that ability if it simplifies the code.

I'm pretty sure we support that.

@Crell do plugins usually have a UI as well? That seems to be the points you made earlier, trying to wrap my head around this. The direction this is going looks really good.

Kris and I disagree about this often, but my general rule is "Should a USER, through the UI, be adding/removing/reconfiguring this, or a developer?" If a user is picking which Thingie(s) to use, then a Plugin is a good fit (although not everything that applies to is, necessarily, a plugin). If a developer is doing so, it should be a service. (This rule is not 100% absolute, but I'd say it's 90% or better.)

Picking which theme engine a given theme uses is very much a developer-task; it's not something a button-pusher will ever be deciding. Thus a theme engine should be a service, not a plugin.

Thanks for the response. And does Plugin work better for the case we have here where each theme can have a different engine? We do support this like @dawehner mentioned in #19, though may need more tests to this likely. We should be able to get classy, stable or core's markup if we have a theme using a different engine.

I don't think that changes anything. If we have each theme engine registered as a service with a different name, and each theme simply references that engine by name (give or take some name munging if we want), then it's a non-issue. It allows multiple engines to be loaded at once (since they're just services), and a given theme can call out to that however it wants.

It's really kind of liberating to get into a services mindset. :-)

One thing we need to keep in mind during this discussion. Theme engines are already extensions, much like modules as themes are,
so there is not yet a clear defined path how to got from there to the plugin/service you want to use. Maybe theme engines would just provide their own theme engine service.

So stepping back: What are we trying to solve? What's the bug?

Conceptually, moving engines to be just a service provided by a module sounds great to me but is not doable in D8 for BC reasons. So... what's broken that we need to fix? Or was this just a "I wonder if" that we never got to, and should probably just punt on?

Seems to me to be cleanup and organization. That's why it's marked as a Task. Less global/variable functions laying about and more unit testable code. From my naive point of view anyway...

As one of the primary authors of the plugin system, Crell's comments in 21 and 23 ring true for me. 2+ years ago I supported this being plugins, but a lot has changed in the theme layer since that time, and the notion of a theme's info.yml file simply stating which service is its theme engine seems sensible to me so long as we have an appropriate instanceof check/typehint involved.

As I am agreeing with Crell, I also want to point out that Plugins are great for when you only want a particular class of object returned no matter what. A plugin manager only returns plugins of a type, that can be locked down to a particular interface, that's nice. What's being suggested here opens up ANY SERVICE for reference. That's not necessarily a bad thing since again, instanceof/typehint prevents any unwanted side effects of this, but it's worth mentioning.

Another point I'll make here is that theme engines (well twig at least) have a bunch of dependencies. Having them be raw services makes more sense in this regard. Pretty sure I support a "theme engine: some.service.id" style solution here.

Eclipse

Cool, we have some agreement!

Retitling...

I'll give this a try

Alright, what this would require us:

• Expand the DrupalKernel to allow theme engines to register services
• Given that it might be needed to have something like install/uninstalling of theme engines
• The alternative is to write some kind of service provider which uses the yaml file loader to read in those .services.yml files

Another round of questions we need to talk about:

• Does the interface look as expected?
• How do we handle the BC layer
• One day: We define one service, when the theme engine not yet defines one, which calls out to the old functionality ...
• For preprocess functions should be able to execute services in general, so we can leverage that for theme engines, but this makes part of the code coupled to services, sadly

For now this is just a quick start, we should discuss about that.

Rerolled patch #31.

Setting back to "Needs Review" state to trigger the test bot.

Only reroll #39 patch for 8.7.x

I could apply the patch, so no reroll is needed.

There's more changes then _init() hook removal required for new interface

Thinking a bit more about implementation the engines should be tagged services instead of relying on service name

Sounds like duplicate of #2973439: Make extension list services tagged services

Updated and converted this to a MR. This was tricky because not a single line of this still applied cleanly, at the same time, various things are also no longer necessary and much simpler now. We already have the callback resolver in ThemeManager, initTheme no longer exists.

I had the idea to deprecate preprocessing for theme engines. It doesn't seem used anywhere and this already doesn't support per-hook preprocessing. I suppose we could keep the generic one if we can think of a use case, but I'd much rather move it out if not needed, simplifies the interface.

Fully agree that this should use tagged services inject them, but wanted to start somewhere. I guess the collection class was kind of meant to do that, unsure if we want that in a separate service or inject it into the ThemeManager directly. We do need it in the Registry as well, but we have access to the theme manager there.

I'm also wondering if we want to deprecate theme engines as a type of extension completely. If they're a service then a module can provide a theme extension. A theme could as well if we support services there.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I was wondering how this was actually working. The answer is, it wasn't. theme engines have no services, the services file name and typos and the content wasn't valid either.

I think the sensible path forward is indeed to completely deprecate the concept of theme engine extensions. theme engine services must be provided by modules. That means existing theme engines will need to be completely redone as a module, but most that I could find are just experiments/old. experience_builder seems to be doing some stuff with it.

This is so good!

Let's get the summary updated, the CR started etc.

I think the function deprecations linking here is ok.

I have one question I'm not sure about how collectors work exactly I guess.

A couple architectural questions we need to resolve.

A few minor clarifications.

We might need to search documentation to update how theme engines are created too.

I created a start for the CR as well.

If we decide to deprecate in a follow up we can move it there.
I think we should do the full deprecation there since I think it's not much more, it's just the preprocess collection and the .engine discovery.

I updated the param doc so tests will run

@longwave I think more changes are needed if we take that direction, the removed getThemeEngine method is used by Registry which is causing the failures afaict.

Ran out of time yesterday so I pushed and hoped, should get time to fix it up later today.

Should we open a related issue to deprecate "base theme engines" - this doesn't seem used anyway and now theme engines are services they can just extend a parent service.

edit: oh, these are engines for base themes; but I don't see why we need to differentiate them really, they're just theme engines.

> edit: oh, these are engines for base themes; but I don't see why we need to differentiate them really, they're just theme engines.

Possibly not, just like preprocess for theme engines, I think it was originally just a copy paste from themes/base themes without thinking about it too much. IMHO definitely out of scope and I'd also wait until support for theme engine preprocess is gone. We several BC layers now in ThemeManager and the Registry and I'd postpone too much refactoring there to D13 when we can drop these, things should get simpler then.

Added a deprecation for .engine files.

I'm confused by the second half of ::loadActiveTheme(), when does getEngine() return null/empty string and trigger this code?

    else {
      // Include non-engine theme files.
      foreach (array_reverse($active_theme->getBaseThemeExtensions()) as $base) {
        // Include the theme file or the engine.
        if ($base->owner) {
          include_once $this->root . '/' . $base->owner;
        }
      }
      // And our theme gets one too.
      if ($active_theme->getOwner()) {
        include_once $this->root . '/' . $active_theme->getOwner();
      }
    }

I guess we also should deprecate ThemeEngineExtensionList while we're here?

Started looking into ThemeEngineExtensionList aka extension.list.theme_engine.

It's injected into extension.list.theme where it's used to populate the owner and prefix properties of theme value objects. owner is the pathname to the .engine file, which will go away once these are services, and prefix is the engine name itself, but I don't see where this is actually used so hopefully it can just be dropped.

It's also injected into extension.path.resolver (as are all the extension lists) but I don't think the engine list is ever used in core from here. I think extension list services should be similarly converted to tagged services and then the path resolver can pick them up using a service locator; that way, contrib could add another extension type if they really wanted to (though this seems super edge cases) but also the lists will be instantiated on demand instead of at injection time. Will open a sister issue to handle this.

Oh, #2973439: Make extension list services tagged services already exists for similar reasons to #81.

Following #81 the prefix property on theme value objects is used in a couple of tests, and more importantly ThemeSettingsForm, where we have another magic function name:

    if ($theme) {
      // Call engine-specific settings.
      $function = $themes[$theme]->prefix . '_engine_settings';
      if (function_exists($function)) {

I think we can just drop this with no replacement? I had no idea this existed, and if there are custom theme engines somewhere in the wild then they can alter this form in their new custom module instead now.

By drop it with no replacement you still mean with a deprecation notice right?
I think we also need to do the same in preprocess in two cases since engines explicitly support preprocess for $name .'_engine' and the theme preprocess.

I think we want to drop both of those behaviors since again we have alternatives.

This is getting super tricky to deprecate extension.list.theme_engine while still actually needing to use it for various tests, maybe have to revert this and leave it as an unused service that we can just quietly remove in Drupal 13.

> This is getting super tricky to deprecate extension.list.theme_engine while still actually needing to use it for various tests, maybe have to revert this and leave it as an unused service that we can just quietly remove in Drupal 13.

That's what I expected and didn't even try. My vague idea was that we'd just leave the extension list in place, it won't find anything anymore in D13 as engines are no longer supported and we could either deprecate it for D14 or remove it immediately.

#3047289: Standardize how we implement in-memory caches has a similar chicken/egg situation with the cache.static service. We need to access it as long as it still exists for invalidation. @catch suggested to then ignore the deprecation message, but I don't really see a difference to not have a deprecation in code. We could also just have a change record without forced deprecation? I'd imagine that this service is very, very rarely used, there's no reason to interact with it directly.

Removed ThemeEngineInterface::getExtension(), the theme engine is directly responsible for appending file extensions now.

Otherwise, are we done here? Or is there more preprocess to deprecate somewhere?

Some final comments from me, I agree this is ready, but I'd prefer if someone else RTBC'd it, while @longwave made considerable changes, much of the underlying idea and approach was done by me.

Fixed up a couple of comments and added types to ::theme(). Added one more bikeshedding comment!

It's on my list!

I think this is ready.

Had a quick discussion on slack about the removal version and got agreement that 12 is ok.
Also discussed preprocess and noted that it's more of a soft deprecation here so added a comment in the CR.

Beyond that I've been through this several times and have been following closely since 65.

There is one open thread but I think it's out of scope for this issue.

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR and added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

Thanks!

This likely would not have caused a problem since the test was pre-existing and had the attribute on head, but getting the deprecation bit in helps confirm we didn't miss any.

Oh, really? It looked like core/modules/system/tests/src/Kernel/Theme/ThemeEngineTest.php was brand-new. My mistake.

No you're right it was missing! I thought I had double checked.

But yes this is very helpful cause it's super easy to miss.

How are you finding issues that are missing these attributes?

How are you finding issues that are missing these attributes?

Brute force. There were only 111 RTBC issues this morning. I opened them all and skimmed their MRs. It took maybe 20 minutes (not counting the time to update the ones with missing attributes).

Added a question about private Vs protected, I think we perhaps have a valid case for a private property here.

This needs a rebase now that the theme oop issue is in. The refactoring I did there should make it easier to trigger a deprecation on theme engine preprocess functions which I just see now we don't have yet as only those still us the prefixes loop.

Rebased, but added some self review comments; not sure what to do about BC here.

Reviewed the rebase, looks good!

I provided an admittedly ugly workaround for your bc question, but I think it might work.

Not a huge fan of private in general, but won't push back here.

Discussed with @nicxvan and @catch in Slack. To provide BC I thought I had a way to add a temporary service with !service_locator that could then be pulled from the container, but without also adding a temporary proxy/wrapper class I don't actually think this is possible. Given that extending ThemeManager seems unlikely we decided to go ahead here without the BC layer.

I think this is ready again!

I saw you tried to add the deprecation, but that caused a of deprecations, the pipeline on the version where only deprecated theme engines never completed.

The problem is that this logic with the theme prefix is not OOP aware. Either it's not needed or it's something we need to convert. We run through themes again a second time a below, I suspect with that, this might be completely necessary or even harmful because it also won't consider legacy hooks, so might add both legacy and OOP preprocess for themes that rely on that. I think we have no test coverage for LegacyHook on preprocess for themes?

We can investigate in a separate issue as it's not directly related to this, but I think we should see what happens when we just comment out $prefixes[] = $theme;. My guess is nothing breaks. I only did a quick manual test but confirmed that nothing breaks visually on olivero frontpage, the later loop catches some manually verified preprocess functions and they're still called.

We could add something like this: (new \ReflectionFunction($function))->getAttributes(LegacyHook::class) and or add a check if it's already in the list of preprocess. I think this already existed though, it's not new here so we can probably do this in a follow up like @berdir suggested.

However, we got the ok to do the deprecation and removal in 12 per 95. I'll add some suggestions for that.

I opened #3554919: Stop discovering theme preprocess twice for theme engine discovered templates, it's green.

Added some comments to the MR.

Thanks for the review, addressed all feedback. Really hoping we can squeeze this into 11.3 or we likely have to wait until 12.1...

Changes look right and all feedback has been addressed!

Committed and pushed 2819baaa300 to 11.x and 6147b655304 to 11.3.x. Thanks!

This caused a fatal error on 11.x for Canvas which has a semi_coupled theme engine.

Is there a BC layer that should work until any existing engine has a chance to update?

Error 500: Service "semi_coupled" not found: the container inside "Drupal\Core\Theme\ThemeManager" is a smaller service locator that only knows about the "twig" service.

Yes, there should be, the BC test just verifies the initialization and not the theme manager/ rendering

Trying to go forward here - the reason for the BC issues is #3556130: ThemeEngine BC layer is broken - ServiceLocator != Container