Due to #1497366: Introduce Plugin System to Core successfully commited can we talk about migrating template system into plugins?

Files: 
CommentFileSizeAuthor
#31 1685492-31.patch13.67 KBdawehner
#15 theme_engine-1685492-13.patch17.4 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,697 pass(es), 38 fail(s), and 34 exception(s). View
#13 theme_engine-1685492-13.patch17.4 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,700 pass(es), 38 fail(s), and 33 exception(s). View
#8 theme_engine_plugins-1685492-8.patch15.11 KBdawehner

Comments

jenlampton’s picture

Status: Active » Postponed

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. :)

EclipseGc’s picture

Issue tags: +Plugin system
jenlampton’s picture

Project: Drupal 8 with twig - abandoned sandbox »
Issue tags: +Twig engine

moving to the active twig sandbox.

c4rl’s picture

Title: theme engine as plugin due to new plugin system » Theme engine as plugin due to new plugin system
Component: Code » Twig engine (twig_engine branch)
sun’s picture

sun’s picture

Project: » Drupal core
Version: » 8.x-dev
Component: Twig engine (twig_engine branch) » theme system
Category: Feature request » Task
Priority: Minor » Normal
sun’s picture

Title: Theme engine as plugin due to new plugin system » Convert theme engines into plugins
Related issues: +#304486: Theme Engines as Modules

Better title.

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

dawehner’s picture

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
Crell’s picture

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.

sun’s picture

@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.

EclipseGc’s picture

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

dawehner’s picture

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.

dawehner’s picture

Status: Active » Needs review
FileSize
17.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,700 pass(es), 38 fail(s), and 33 exception(s). View

Continued the work and got the installer working with it.

Status: Needs review » Needs work

The last submitted patch, 13: theme_engine-1685492-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,697 pass(es), 38 fail(s), and 34 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 15: theme_engine-1685492-13.patch, failed testing.

Cottser’s picture

Issue summary: View changes
Related issues: +#1804864: Make theme engines classes
Crell’s picture

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.

dawehner’s picture

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.

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev

@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.

Crell’s picture

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.

joelpittet’s picture

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.

Crell’s picture

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. :-)

dawehner’s picture

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.

Crell’s picture

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?

joelpittet’s picture

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...

EclipseGc’s picture

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

dawehner’s picture

Cool, we have some agreement!

Crell’s picture

Title: Convert theme engines into plugins » Convert theme engines into services

Retitling...

dawehner’s picture

Assigned: Unassigned » dawehner

I'll give this a try

dawehner’s picture

Assigned: dawehner » Unassigned
FileSize
13.67 KB

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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.