Deprecate theme_get_setting()

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 🌱 [policy] Standardize how we implement in-memory caches Needs work

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 🌱 [policy] Standardize how we implement in-memory caches Needs work 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?