Updated: Comment 0

Problem/Motivation

The theme negotiator issue allowed us to determine the active theme on a request, though it did not replaced all the global theme calls.

Proposed resolution

This patch tries to replace most of the global $theme/$theme_key places, though it is tough as it is also used in order to figure out whether the theme system is already initialized.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
12.34 KB

Just started with a patch here.

Status: Needs review » Needs work

The last submitted patch, 1: theme-2167619.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.33 KB
446 bytes

ups.

The last submitted patch, 3: theme-2167619.patch, failed testing.

larowlan’s picture

Issue tags: +theme service
dawehner’s picture

Let's see what a reroll tells us.

Status: Needs review » Needs work

The last submitted patch, 6: global_theme-2167619-6.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
21.52 KB

This allows me to install drupal via the UI.

Status: Needs review » Needs work

The last submitted patch, 8: global_theme-2167619-8.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
23.77 KB
2.4 KB

This currently logs out the user immediately after running the tests.

Status: Needs review » Needs work

The last submitted patch, 10: global_theme-2167619-10.patch, failed testing.

www.cherritvandermerwe.com’s picture

is there any new patches?

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.37 KB
6.16 KB

The installer did not worked due to a circular dependency.

Status: Needs review » Needs work

The last submitted patch, 13: global_theme-2167619-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.59 KB
638 bytes

Another madness:
"date -> entity.manager -> module_handler -> theme.negotiator -> access_check.theme -> theme_handler -> config.installer -> config.manager".'

Status: Needs review » Needs work

The last submitted patch, 15: theme_global-2167619-15.patch, failed testing.

sun’s picture

  1. Why do we need to change all those service definitions here?

  2. I actually wonder whether ThemeNegotiator is the right service to consult.

    And/or, in general, I've the impression we have at least one or two too many theme services right now. Yes, all of them have a discrete purpose, but because they are decoupled, the theme is no longer synchronized between them.

    Synchronization is the exact purpose of a global variable: It is guaranteed to have the same value, regardless from where or how you access it.

    The lack of synchronization means that the following scenario is completely possible right now:

    Service A negotiates theme foo, but service B checks access to theme bar, service C renders content with theme baz, and service D doesn't even have themes foo, bar, and baz in the list of enabled themes. WTF? :)

    Thus far, we've been treating the theme system similar to the module system architecture, which makes some level of sense, but when it gets to the actual runtime behavior, then the theme system diverges in significant ways from the module system:

    There is only one theme at a time. In order to switch to a different theme, a whole bunch of related and dependent services need to be re-initialized for a different theme.

    [whereas that almost reminds of a Container scope...]

    IMHO, the next best step is to merge and consolidate various classes and services into a single:

    1. Theme negotiation + access + initialization are extremely tied to each other → that's one and the same thing. It's whatDoWeWantToUseToday() + canIUse() + use().
    2. Theme\Registry::init() must die → it manually initializes a theme out of nowhere (not even checking for access nor validating anything else).
    3. ThemeHandler is a base dependency for the consolidated service in 1) → it supplies and controls which options are available in the first place.
    4. But even ThemeHandler and 1) can get out of sync way too easily → upon every single change to ThemeHandler::$list, 1) MUST be re-instantiated.

    → Consequently, we should honestly ask ourselves if 99% of the functionality shouldn't be moved directly into ThemeHandler.

dawehner’s picture

I can understand that theme negotiation, access and init is kinda dependent on each other, Registry::init() is horrible but I don't get why this united service can't just always ask the theme handler for the list of themes.

sun’s picture

dawehner’s picture

Status: Needs work » Fixed

The other issue takes care of this.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.