We're looking into ways to let themers define their own Twig filters and functions, and one way to do it would be to allow themers to define their own Twig Extensions within their themes. This ought to be possible using a services.yml file, together with a patch such as the one at #1964156: Contrib cannot define additional Twig extensions, but it looks like these files are only supported by modules right now.

Is there any indication that services.yml files might be supported by themes?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markhalliwell’s picture

markhalliwell’s picture

Status: Active » Needs review
FileSize
15.57 KB

Nope, that issue is unrelated. Here's a patch that will hopefully fix this issue

Status: Needs review » Needs work
Issue tags: -Twig, -theme system cleanup, -Stalking Crell

The last submitted patch, drupal-themes-dont-support-2002606-2.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Twig, +theme system cleanup, +Stalking Crell

The last submitted patch, drupal-themes-dont-support-2002606-2.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
13.43 KB

Themes don't have to specify a THEME_NAME.theme file, which caused certain arrays to be empty and throw a PHP error. Also removed coder cleanup since it's not related to this patch. Also removed profiles from the theme array ;)

Status: Needs review » Needs work

The last submitted patch, drupal-themes-dont-support-2002606-6.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
13.6 KB

Fixed so test would validate

Status: Needs review » Needs work

The last submitted patch, drupal-themes-dont-support-2002606-8.patch, failed testing.

markhalliwell’s picture

I give up, I can't for the life of me figure out why this last patch is failing. It's 5 small fails to seemingly unrelated tests. If anyone can please shed some light onto this?

markhalliwell’s picture

Title: Themes don't support their own services.yml file » Theme *.services.yml files aren't detected
star-szr’s picture

The failures on the latest patch are all locale related. Not sure if that helps though.

kfritsche’s picture

I tried to help Mark Carver here as asked in I18N IRC channel but after a long night of debugging I couldn't figure it out.

At least reporting the status.
I can't reproduce this when adding a new language, adding the config for the site name by hand and loading the page. It works and it is what the test does.
So I had to run the test all the time, as it seems for me the only way to reproduce the issue... But debuggin in tests is a little bit hard.
Either way the problem is somehow in the LanguageManager.php. The getLanguage() Method is returning sometimes the wrong language. I added many debug messages in this class and the output was similar to:

...
this->languages[language_interface] = en #1
reset() #2
this->languages[language_interface] = xx #2
this->languages[language_content] = xx #2
this->languages[language_url] = xx #2
isset(language_interface) return xx #2
isset(language_interface) return xx #2
notify load system.site (EventDispatcher)
isset(language_url) return xx #2
isset(language_interface) return en #1
isset(language_interface) return en #1
notify load system.site (EventDispatcher)
isset(language_url) return xx #2
isset(language_interface) return en #1
isset(language_interface) return en #1
isset(language_interface) return xx #2
isset(language_url) return xx #2
isset(language_interface) return xx #2
isset(language_interface) return xx #2
...

Just in the middle it returns from the $this->languages a different value then it sets before. I used spl_object_hash($this) to also add the hash (cutted above) of the objects. There are somehow two different LanguageManager initializiesed. #1 which has wrong values and #2 which works. I have no clue how THIS patch introdoces this behavior. But without the patch everything is working and its always the same hash.

Hopefuly it helps somebody to fix this issue as it took me the complete night to figuring this out.

catch’s picture

I'd prefer to leave services to modules, at least until #1067408: Themes do not have an installation status. Without that this seems like opening a can of worms. A theme can always have a helper module that does the Twig extension.

catch’s picture

Status: Needs work » Postponed
markhalliwell’s picture

At first I thought the same, but I really don't think it makes much of a difference. This patch only detects enabled themes and adds them to the Kernel, along with any services they might want to register. Granted they might not be properly installed (per #1067408: Themes do not have an installation status) but I was able to successfully detect and register a service in a test theme.

I also respectfully and strongly disagree that only modules should provide services. #1964156: Contrib cannot define additional Twig extensions, allows Twig extensions via a service and I'm sure there are other "services" that will come to pass, could greatly enhance a theme's capability.

A theme can always have a helper module that does the Twig extension.

Doesn't this kind of defeat the whole purpose of putting Twig in then? Twig was introduced to help move theming TO themes. It seems rather counter-productive to not have themes define their own functions/filters that's specific to their theme. Plus I would really like to get away from "oh btw, to use this theme you have to also install X module". Why can't themes work out of the box?

An example of this would be any theme that uses a grid system. In Bootstrap, this would help replace a lot of the Bootstrap specific functions that were used in templates, ie: determining the spanX widths of the sidebars and content, since they vary. Yes... if it really came down to it, we could do all the calculations in hook_preprocess_page() and add extraneous variables to accommodate all these variations, but that seems silly and definitely doesn't feel like the right approach.

markhalliwell’s picture

Also, considering #1964156: Contrib cannot define additional Twig extensions is marked "critical", I'd almost argue that this issue goes in tandem with we're trying to do with Twig in general.

markhalliwell’s picture

Ok, been thinking about this and I can see how allowing any service could be detrimental. What if we were to parse the theme's services.yml file and only allow white-listed services? Any others that are detected in that file would throw and error saying that it needs to be taken out.

markhalliwell’s picture

Ok talked to msonnabaum on IRC and he suggested that we move twig extensions out of services.yml and put the twig extension in the info.yml instead.

msonnabaum:

the service container is not an appropriate concept at the theme layer

twig.extension could be used to house the name of the class to use for extending twig in the info.yml file. I'm wondering if we can do this for both modules and themes then and refactor #1964156: Contrib cannot define additional Twig extensions to look for it in the info.yml instead of services.yml.

sun’s picture

Issue summary: View changes
Status: Postponed » Active
Parent issue: » #2228093: Modernize theme initialization

From an architectural perspective, Twig extensions are "plugins" in Drupal's architecture.

If we'd (1) keep on registering the theme namespace and (2) discover twig extension plugins in each theme (and its base themes), would that be a potential solution?

For example, assuming a custom domain plugin namespace/directory (sans \Plugin):

/themes/bootstrap/lib/Drupal/bootstrap/Twig/MyExtension.php

Once the switch to PSR-4 has landed, that would become:

/themes/bootstrap/src/Twig/MyExtension.php

Thoughts?


Note: If those twig extensions are compiled/dumped in anyway, then they MUST be compiled into non-global, theme-specific PHP file, because the current/active theme can change at any time. — But I hope that this is not the case in the first place?

markhalliwell’s picture

Priority: Normal » Major
Related issues: +#1886448: Rewrite the theme registry into a proper service

Considering that the registry is actually now a service, this is a necessity and kind of a major issue:
A [base-]theme that needs to do registry processing/altering should be able to subclass core's methods in a service.

catch’s picture

Status: Active » Postponed (maintainer needs more info)

I'd rather see us allow shipping modules within theme project directories, and have the module provide the services. iirc that actually works now already.

Don't see how providing services in themes can work - the theme can vary arbitrarily, but the container gets compiled once per site.

sun’s picture

@catch: I agree with the basic direction. The only legit OO code in a theme would be Twig extensions in my book. Ideally, we'd drop support for OO code in themes.

But I still don't know whether making "themes can ship with modules" official is a safe/good idea — it's an unintended side-effect of the extension system cleanups/performance improvements that I didn't think of. If we want to make that official, then let's create an issue to add test coverage + think through possible consequences.

jhedstrom’s picture

Status: Postponed (maintainer needs more info) » Closed (won't fix)

Closing per

Don't see how providing services in themes can work - the theme can vary arbitrarily, but the container gets compiled once per site.

markhalliwell’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Feature request
Status: Closed (won't fix) » Active
Related issues: +#1685492: Convert theme engines into services

While an active theme can indeed vary arbitrarily (based on any number of criteria), adding installed themes to the container doesn't mean we have to invoke a *.theme file does it? I'm not suggesting that we make this mandatory for themes, it would purely be opt-in.

Allowing a theme to extend/replace the theme registry service, create twig extensions and now potentially specify a theme engine (see related issue).... these are all (will be) services. What will come next???

It seems to me, that it makes more sense to allow themes to participate in services rather than creating yet more boilerplate code to monkey patch the current "theme system".

If need be, we can limit it some way like we did with alters (I don't like that idea personally, but trying to be a team player here and compromise). The point is is that we have to deal with the ever changing plugin/services/container system that Drupal has become. We cannot continue treating the "theme system" like it's not part of this great and powerful system.

It's chastising [base] themes that have resort to, essentially, straight up hacks via alters (what little are left, meh) to do what should be done as a proper extension/replace of a service: http://drupal-bootstrap.org/api/bootstrap/src%21Plugin%21Alter%21ThemeRe...

Just because most themes won't take advantage of the power of PHP/core... doesn't mean the ones of us who want/need to should suffer.

---

I know some people think that theme's "shouldn't do stuff like this [OO, services, etc.]", but they're wrong.

The reality is:

It's not that they "shouldn't do it". It's that they likely won't need to do it. This doesn't change the fact that some [base] themes might actually need certain abilities, like native OO (namespace discovery?).

Namespaces is native PHP functionality.. why would we suggest removing discovery of that? WTF. I'm not sure why there's this mentality in the community that "oh themes won't need this or that... just remove it". This is wrong... please stop.

darol100’s picture

#25 I agree with you 100%. 1+ for this feature.

straight up hacks via alters (what little are left, meh) to do what should be done as a proper extension/replace of a service

There is an issue in DrupalConsole (Only Command classes loaded from themes ) to try to replicate a similar hack of what Bootstrap project is doing to load the classes.

@markcarver, it will be nice to get your input in that issue.

catch’s picture

Title: Theme *.services.yml files aren't detected » Allow themes to provide services.yml
Issue tags: +Needs issue summary update

While an active theme can indeed vary arbitrarily (based on any number of criteria), adding installed themes to the container doesn't mean we have to invoke a *.theme file does it? I'm not suggesting that we make this mandatory for themes, it would purely be opt-in.

What happens if you have front-end and admin themes using two different base themes, and both of them are trying to differently override the same service? Only one can win, but themes traditionally think they have total control without interference from other themes installed.

If need be, we can limit it some way like we did with alters (I don't like that idea personally, but trying to be a team player here and compromise).

With alters, everything that invokes the alter has to add the current theme to the cache ID to avoid conflicts between different active themes. With services we can't do that, so we'd be doing services globally, but alters per-theme. It feels hard to document what the behaviour should actually be.

These questions were not asked when we originally made drupal_alter() work for themes, then we spent three+ years dealing with unexpected behaviour (hence why we had to limit it to specific alters later). So my main concern is we think about the consequences prior to adding the feature.

Similarly, when we made profiles a special type of module back in 5.x or 6.x, that also led to dozens and dozens of issues with the extension system that still aren't resolved either (such as dependencies[] meaning two completely different things). Because module functionality just got bolted on to something that was not really a module.

Another option that's been suggested (but I'm not sure if there's an issue, or where it is if so) is allowing themes to ship a module (in the same project folder), which they then depend on. Then the module is always-on (because it's a module), and the theme is still 100% based on the request. If two themes ship two modules and they conflict with each other, then there's hook_requirements() and similar to handle that.

So you'd have:

themes/my_theme/my_theme.info.yml (type = theme)
themes/my_theme/modules/my_theme_helper/my_theme_helper.yml (type: module)

And module scanning gets updated to scan all possible theme directories too.

That way the theme would get access to everything that modules do, because it actually would be shipping a module, and we don't have to worry about autoloading etc. because that would all be handled by existing infrastructure too.

While that potentially has its own complications, it seems more straightforward to me, and a module only needs a .info.yml so there's really not much of an extra step.

catch’s picture

OK found the issue #2390973: Allow themes to contain companion modules and depend on them, which @markcarver marked duplicate of #474684: Allow themes to declare dependencies on modules since it looks like that half-works already.

If we fix that issue then 'theme projects' can define services.yml - even multiple if they wanted to.

Then this issue would be explicitly about adding services support to the theme extension itself, not the project. If we can get the feature the first way it seems a lot simpler to maintain, and I'd really want to understand exactly what's wrong with it that this issue would need to happen as well.

davidwbarratt’s picture

+1 for #474684: Allow themes to declare dependencies on modules I would much rather see that then only being able to add twig extensions. Depending on modules gives themes a tremendous amount of power, while still maintaining a separation of concerns.

darol100’s picture

1+ for #27, it seem to be a safer solution.

davidhernandez’s picture

Another option that's been suggested (but I'm not sure if there's an issue, or where it is if so) is allowing themes to ship a module

+1 I was going to suggest the same thing. Use the plumbing we already have.

catch’s picture

Status: Active » Postponed

Not marking as duplicate just yet, but I am going to mark it postponed - let's see what happens over there.

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.

Fabianx’s picture

I am going to put this back to active and provide a very simple patch, too.

I think we have overthought this a little bit too much.

- If a theme is installed it should be able to provide services regardless if its active or not. BUT it should be installed.

In the ideal case the services are namespace'd to the theme, e.g. bootstrap.twig_extension.

Regarding the conflicts:

If theme A was shipping a module and theme B was shipping a module, too and both are not namespaced, then the _same_ conflicts arise with admin vs. normal theme.

So the going with modules route solves exactly nothing - because that two themes depend on the same module means that they need to be both compatible to that module, too and if that happens is unclear.

I think it would be wiser to educate the theme authors to namespace carefully and know that they will be always active - when installed.

My main concern was to only enable certain functionality when the theme is _active_, however since themes now have an installation status, I think it is fine to register it's services when it is installed.

--

Of course the patch is just a quick POC, but given how ExtensionDiscovery has changed (Yeah!) it is now simpler than ever.

For a real clean patch it likely is a good idea to rename moduleList to extensionList, but besides that it looks pretty easy to me.

Status: Needs review » Needs work

The last submitted patch, 35: allow_themes_to_provide-2002606-35.patch, failed testing.

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.

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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.5.x-dev » 8.6.x-dev

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

Pol’s picture

Rerolling the last patch against Drupal 8.5.x.

Status: Needs review » Needs work

The last submitted patch, 40: d8-2002606.patch, failed testing. View results

joelpittet’s picture

markhalliwell’s picture

Drush basically said to fix core and I agree with them: https://github.com/drush-ops/drush/pull/3089#issuecomment-364240707

I really don't think we should be opening up themes to services anymore.

A lot has changed (for the better) in the 5 years I've been following this issue (closely).

If this were to happen... there wouldn't be any real delineation between what a module is vs. what theme is, would there?

Closing in favor of many other issues that will help modernize the theme system #474684: Allow themes to declare dependencies on modules, #2869859: [PP-1] Refactor theme hooks/registry into plugin managers, #2954562: [PP-2] Create provider based plugin managers and ultimately allow us to provide support for selected "services" in other OO ways.

leymannx’s picture

Quite a big thing IMO. But I like the compromise of letting themes declare dependencies on companion modules. Hope we are getting this into core soon.