Deprecate theme_get_setting()

Interesting, maybe instead of theme handler use approach from #3015812: Introduce new Theme extension object and properly deprecate REGIONS_VISIBLE and REGIONS_ALL

I mean theme extension method

Looks really strange, maybe something with static cache or serialization... but as I see no test groups affected so no idea, I will try to debug locally tonight


runtests --list
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "theme.manager", path: "theme.manager -> theme.negotiator -> access_check.theme -> theme_handler -> theme.manager". in /home/berdir/Projekte/d8/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:67
Stack trace:

That's the reason ;)

This topic is... complicated.

Historically, theme settings became config when variable_* functions went the way of the dinosaurs, but it wasn't fully flushed out properly and really only implemented very basic config implementation/support.

When 8.0.0 was released, we had numerous issues (and still do) with ThemeSettings as a whole. It has gotten better over the years, but it has always been a PITA.

Specifically, we still have some issues around proper inheritence/overrides and the ability to (easily) save settings programatically. This is what led to the Drupal Bootstrap base theme implementing its own wrapper around core's (albeit, becoming less and less of an necessity).

I imagine that, yes, to fix this properly, some of this would likely become part of a sub-issue of #2869859: [PP-1] Refactor theme hooks/registry into plugin managers and ultimately live there.

---

For now, if the goal is to simply "clean-up" proceedural functions, the existing procedural code could be moved to ThemeSettings::__construct and it should likely be extended from \Drupal\Core\Config\Config and not \Drupal\Core\Config\ConfigBase.

That being said, we really shouldn't put this in ThemeHandler; it is already a dumping ground for theme related code that should really live elsewhere.

The retrieval functionality should be moved to a the new Theme object that's being proposed as the settings are theme specific.

A single settings() helper method to retrieve an an instance of ThemeSettings (locally cached as a protected property, but not serializable to the DB), which could allow us to retrieve and manipulate settings from the config object as needed, e.g.:

// Assuming $theme is an instance of the new Theme extension object.
$value = $theme->settings()->get('my.theme.setting');

$theme->settings()->set('my.theme.setting', $value);

For this reason, I'm postponing this on #3015812: Introduce new Theme extension object and properly deprecate REGIONS_VISIBLE and REGIONS_ALL.

Also complicating the theme space is the fact we have an object called ActiveTheme which is a pseudo enhanced theme extension object.

In a way the current implementation of theme_get_setting() is as a factory for ThemeSettings objects. Maybe a good design would be to introduce a theme settings factory to the container. We can generate services based on installed themes in a compiler step. So \Drupal::service('theme.settings') returns you the active theme's ThemeSettings object and \Drupal::service('theme.settings.THEME_NAME') gets you the object for a specific theme.

Also complicating the theme space is the fact we have an object called ActiveTheme which is a pseudo enhanced theme extension object.

Yes, I was thinking about this too. Off topic, but would it make sense to extend it from the new Theme extension object once it's in? IIRC, its primary purpose was a slimmed down Extension object so it could be stored in the DB. I feel like we could accomplish the same effect by overriding the __sleep method so it only keeps what we need. It has always been a PITA having to have special logic to deal with ActiveTheme objects opposed to Extension objects (or both).

Maybe a good design would be to introduce a theme settings factory to the container.

I'm a bit torn on this, so bear with me while I semi-rubber duck my thoughts here.

Considering that this is just a wrapper for theme specific config, and it's only ever relevant to that specific theme, I think on one hand this is a bit overkill. Similar to why you want to keep extensions internal (not sub-classable), this isn't something we likely want to expose (at least not yet, until more of the theme system has been converted into proper OO services).

On the other hand it, in theory, could allow a module to replace/extend the services with their own implementation to provide additional functionality. That being said though... it's config, what more functionality could we possibly need? The only reason Drupal Bootstrap has to wrap ThemeSettings is that it currently only extends from ConfigBase, not Config (i.e. very basic functionality; no saving, overrides, etc.).

I'm thinking that, for now, it may be easier to just fix the last few remaining issues we have with ThemeSettings (extend from Config, fix theme ancestry setting inheritance) and just initialize new instances of it on the Theme extension object.

If we decide that a factory is truly needed in the future (perhaps as part of #2869859: [PP-1] Refactor theme hooks/registry into plugin managers), there's nothing really preventing us from doing that. We could simply do \Drupal::service('theme.settings.' . $this->name) in the Theme extension object instead of manually instantiating a new instance, yes?

@markcarver but the problem is that it is not

just a wrapper for theme specific config,

- it is something that has to be made from several pieces of theme config and theme info file and has to be created at runtime because of the file_url_transform_relative() stuff. This is why for me the factory pattern makes sense.

wrapper for theme specific config

I bet we could use config entity for that with "interfaced" methods to third party settings config

No reason to postpone, mixing regions with settings in config object, entities could allow to reference and depend directly

Here' re-roll for 9.3 and a bit of clean-up

Remove unrelated parts and fix merge conflict

Update leftover implementation

Fix CS

Fix derecation

Message should be simmilar

Fix service

re-roll for 10.1

Patch #34 applies without problems (ON TOP OF #3015812: Introduce new Theme extension object and properly deprecate REGIONS_VISIBLE and REGIONS_ALL) though:
- Settings don't get saved anymore
- We get a nice error:
Deprecated function: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in Drupal\Core\StreamWrapper\StreamWrapperManager::getScheme() (line 256 of /opt/drupal/web/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php)

Also, not sure where/when, the 'favicon' setting for the theme disapperead...

Working on this, fixed #36

I think the main thing here is the service/class name. Otherwise this looks good to go.

With regards to the param types. Part of me wants to say yes lets do it and knock it out. But other part is concerned how much it could expand the scope?

no one else had a vote, lets give it a shot.

Done

Believe that was the last bit.

I think we should consider improving the architecture rather than injecting yet another service into the theme installer. I think we could even remove the clear cache method if we listen to configuration events.

Improved the solution substantially with:

1. Config subscriber as requesting by @alexpott to clear theme settings caches when core.extension:theme changes
2. Memory cache service following recommendations in #3047289: Standardize how we implement in-memory caches

Some things are missing in the CR and the proxy class likely needs to be regenerated.

Also had a question on the new service.

Addressed feedback, service names are personal preference I guess? Personally not a huge fan of a class name service and interface name alias.

Another review.

@berdir wonder if some of the threads from you can be resolved?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

This is very close, as far as I can tell the only remaining task is to move the event subscriber to the right namespace, core/ theme/ eventsubscriber.

Ok, I took another look through this and I think it's ready.

There are three open comments on the MR still, but I think the answers address the concerns. I left them open in case others have a different opinion.

Added some review comments to the MR - we need to ensure uninstalled themes are removed from the memory cache too.

I feel like this is a lot of complexity with that event subscriber, I'd suggest we do one of two things:

a) As mentioned before, ThemeInstaller and ThemeSettingsProvider are in the same component. This is like a module implementing it's own hook/event. We could just do a 1:1 transformation and reset the static cache bin keys directly in ThemeInstaller.

b) Add the core.extension cache tag to those cache entries. Yes, that will reset it on any module or theme install or uninstall completely. But we're talking about module/theme install (or well, weight changes, but how often do those happen?). I still think a any of those actions should just do a full cache reset. That logic is pretty much exclusively there for tests and edge case, I debugged ThemeInstaller when installing a theme through the UI and $theme_settings is NULL, it's going to be the same for drush and config import as well. There's nothing to reset in all regular cases.

@berdir suggested using the core.extension cache tag and then we wouldn't need the listener at all. Makes sense to me.

I've removed the event subscriber and added the cache tag, IIUC that's all that is needed

Like right to me, I'm going to give berdir and alexpott a chance to look since it was their concern and solution.

This looks good to me as well.

My proposal used a named argument, which I think is really neat in this case as we don't need to pass in the extra constant and have a use for that, but AFAIK, it's explicitly excluded by our BC policy, so not sure about that: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic.... Leaving that to @alexpott.

I think we're really close but when reviewing the code I realised we should add the cache tag info to the ThemeSettings object and use that. It'll nicely encapsulate the tags required for the memory cache.

Also we're missing a BC layer for drupal_static_reset('theme_get_setting');. The drupal_static() usage is a little more tricky. Not sure what to do about that.

Also adding cache.memory here is interesting. For other memory caches we've done stuff like

  # Set up a cache chain for asset caching as the same assets may be
  # requested several times, non-public as they are not meant to be reused.
  cache.asset_memory:
    class: Drupal\Core\Cache\MemoryCache\MemoryCache
    arguments: ['@datetime.time']
    public: false

and

  system.module_admin_links_memory_cache:
    class: Drupal\Core\Cache\MemoryCache\MemoryCache
    arguments: ['@datetime.time']

Should we really be adding a generic one? I'm not sure about that.

Obviously something that is interesting about the memory cache backend adding for assets and admin links is that I suspect their entries are not covered by the cache tag invalidator...

So perhaps all that needs changing here is that cache.memory should become a more specific name... inline with

  cache.access_policy_memory:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin.memory, default_backend: cache.backend.memory.memory }
    factory: ['@cache_factory', 'get']
    arguments: [access_policy_memory]

Since alexpott detected my attempts to sneak in the cache.memory bin, we agreed in Slack to revive #3047289: Standardize how we implement in-memory caches and pull that bit out. It's an old issue, but we essentially already did what was originally proposed there, so it _should_ be simpler.

Sounds like we need to postpone this issue then?

#3047289: Standardize how we implement in-memory caches is now in, which means the generic cache bin is in place, and we can rebase this and build on top of that.

Done!

I think this is ready again.

It's using the newly minted memory cache bin.

Still one thread open but I think it's resolved.

Discussed with @catch and we agreed that we should remove the interface. The service is not an important one to be able to swap out or decorate and maintaining an interface implies that it is. When we started on D8 we were a bit interface-giddy… and it felt like there was a rule that every service must have an interface… I know I’ve been responsible for asking for interfaces in the past. But I feel this was a mistake. The class is not final so if you really really swap out you can extend and do that.

Done!

Thanks @nicxvan!

We can'[t remove the underscored function in a minor release. It is going to break sites unless contrib updates... see link. There are others...

Done, new CR added.

No harm in adding it back, but I thought that underscored functions were explicitly excluded from BC, and checking contrib I see one call to it in a drupal 7 module outside of core.

Is that inaccurate?

@nicxvan yes - I linked to a theme actively used in Drupal 11 that uses the function https://git.drupalcode.org/search?search=_system_default_theme_features&...

There are others but that would involving linking to a search tool I don't like to publicise.

Ie. there are at least 50,000 odd sites that would have been broken by this removal... https://www.drupal.org/project/usage/bootstrap

Ah I didn't see the link since the thread was already resolved. Good case for deprecating by default, especially in the core includes I think.

The re-adding back of the _ function is good and the previous rtbc still applies.

Committed be276ec and pushed to 11.x. Thanks!

Glad to see the back of this one, thanks everyone!

Hi
I want to point out that currently the change record is not the same as what this MR did in the core tests.
https://git.drupalcode.org/project/drupal/-/commit/be276ecb985c282019f5c...

Core only invalidates tags, does not delete cache records:

    $this->assertEquals($expected, \Drupal::service(ThemeSettingsProvider::class)->getSetting('logo.url', 'stark'));

The change record deletes a cache record:

// Clear the 'bartik' theme settings static cache.
\Drupal::service('cache.memory')->delete('theme_settings:bartik');

I assume invalidating the cache tag would be enough.
So maybe the change record needs updating?