- Issue created by @berdir
- π¨πSwitzerland berdir Switzerland
Doing the deprecations here feels pretty silly, also for those 3 helper functions that unfortunately aren't prefixed. The chances of those being called by anyone seem pretty much zero.
- π¨πSwitzerland berdir Switzerland
Properly merged with the existing ViewsUIThemeHooks class.
- πΊπΈUnited States nicxvan
Updated the issue summary it was referencing update.
I am reviewing this now.
- πΊπΈUnited States nicxvan
This needs a change record and the cr links need updating for non template preprocess functions and file.
Other than that I checked each deprecation version is correct and consistent.
All initial preprocess are set to the correct hook.
All are __function__
all deprecated functions seem reasonableI have not run the zebra, I will shortly.
Setting to needs work for CR.
- π¨πSwitzerland berdir Switzerland
I decided to just link to this issue instead of a CR because there are no known calls to these functions, not even in D7: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22views..., there really is no reason to call them.
I can do a CR, my goal is to avoid unnecessary noise. There are social media channels, feeds and so on, lots of people will at least scan them to see if it affects them. For me, a CR about these functions is just noise, they should have been _ prefixed, then we wouldn't need the BC layer.
- πΊπΈUnited States nicxvan
I think that's reasonable since it's not used anywhere.
I've resolved my comments and based on 9 this is ready to go!
- π¬π§United Kingdom catch
Similar question as on π Convert template preprocess in system.module Active .
The config factory dependency is only used on one method, so should it be a service closure?
- π¨πSwitzerland berdir Switzerland
I don't really see a scenario where this could be invoked and ConfigFactory isn't already initialized, at which point using a service closure is just overhead?
Currently, ConfigFactory is initialized as part of the dependency chain of \Drupal\Core\EventSubscriber\OptionsRequestSubscriber, a prio 1000 request subscriber (through route provider and path processors). This seems like a good candidate for a service closure as it only needs the route provider on OPTIONS requests.
If I disable that, it's the TimeZoneResolver, a prio 299 request subscriber, that also does the first actual get() call (possibly information that could be put in the container, like language), but there are more actual get() calls.
\Drupal\Core\Theme\DefaultNegotiator::determineActiveTheme (system.theme)
\Drupal\Core\Extension\ThemeHandler::listInfo (core.extension, due to theme status check)
\Drupal\Core\Session\UserRolesAccessPolicy::calculatePermissions (user roles, we removed killed a persistent cache here as that's slower than config get)And that's on mostly warm caches, such as the request matching, inbound path processing involves more config for example.Β¨
system has more services, that's a bit more complicate, I'll check that separately.
Putting back to RTBC to evaluate that again.
- π¬π§United Kingdom catch
No that's a good answer although I'm worried that once we added in more of core and contrib we're going to end up loading a lot of code that's not used.
Committed/pushed to 11.x, thanks!