Updated: Comment 0

Problem/Motivation

In order to get rid of getting the theme system untangled we need a clean way to fill up the globals (which will be somewhere else in the future).
These globals are currently filled in drupal_theme_initialize()

Proposed resolution

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#23 theme_init-2167635-23.patch23.57 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,452 pass(es), 1 fail(s), and 0 exception(s). View
#14 theme_init-2167635.patch19.67 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_init-2167635_1.patch. Unable to apply patch. See the log in the details link for more information. View
#14 interdiff.txt2.33 KBdawehner
#12 theme_init-2167635.patch19.75 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#12 interdiff.txt4.08 KBdawehner
#10 interdiff.txt3.89 KBdawehner
#10 theme_init-2167635.patch18.96 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,861 pass(es). View
#8 theme-2167635.patch16.83 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,582 pass(es), 1 fail(s), and 1 exception(s). View
#4 theme-2167635.patch16.82 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,646 pass(es), 1 fail(s), and 1 exception(s). View
#4 interdiff.txt10 KBdawehner
theme_init.patch14.95 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

Status: Needs review » Needs work

The last submitted patch, theme_init.patch, failed testing.

larowlan’s picture

Combine this with the theme request listener I have in #2016629: Refactor bootstrap to better utilize the kernel and it gets cleaner still, no need to init theme in bootstrap anymore!

larowlan’s picture

Issue tags: +theme service
dawehner’s picture

Status: Needs work » Needs review
FileSize
10 KB
16.82 KB
FAILED: [[SimpleTest]]: [MySQL] 59,646 pass(es), 1 fail(s), and 1 exception(s). View

This should be better.

Status: Needs review » Needs work

The last submitted patch, 4: theme-2167635.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

4: theme-2167635.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: theme-2167635.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.83 KB
FAILED: [[SimpleTest]]: [MySQL] 59,582 pass(es), 1 fail(s), and 1 exception(s). View

Not sure what is going on here.

Status: Needs review » Needs work

The last submitted patch, 8: theme-2167635.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.96 KB
PASSED: [[SimpleTest]]: [MySQL] 59,861 pass(es). View
3.89 KB

This was an interesting ride.

Initialisation gets the ThemeNegotiator injected, so also all negotiators, like the DefaultNegotiator.
That one gets the "system.theme" config file.

If the Initialisation class got initialized before the test code manually sets the default theme, the config object on DefaultNegotiator is not updated.

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/Theme/Initialisation.php
    @@ -0,0 +1,269 @@
    + * Contains \Drupal\Core\Theme\Initialisation.
    

    You're going to hate me but don't we have to use American spellings? Initialization.

  2. +++ b/core/lib/Drupal/Core/Theme/Initialisation.php
    @@ -0,0 +1,269 @@
    +  public function initThemeSystem() {
    

    Needs a docblock

  3. +++ b/core/lib/Drupal/Core/Theme/Initialisation.php
    @@ -0,0 +1,269 @@
    +   * This function is useful to initialize a theme when no database is present.
    

    Can this be expanded on, as the method name doesn't immediately make sense given this comment.

  4. +++ b/core/lib/Drupal/Core/Theme/Initialisation.php
    @@ -0,0 +1,269 @@
    +   * @param \stdClass $theme
    

    Needs a short summary

  5. +++ b/core/lib/Drupal/Core/Theme/Initialisation.php
    @@ -0,0 +1,269 @@
    +   * @param \stdClass $base_theme
    

    Can you elaborate on the expected format of these objects?

  6. +++ b/core/lib/Drupal/Core/Theme/Initialisation.php
    @@ -0,0 +1,269 @@
    +      $theme_engine = $theme->engine;
    +      if (function_exists($theme_engine . '_init')) {
    +        foreach ($base_theme as $base) {
    +          call_user_func($theme_engine . '_init', $base);
    +        }
    +        call_user_func($theme_engine . '_init', $theme);
    +      }
    +    }
    

    General comment: boy the theme system could do with real objects and some interfaces (not related to this patch).

    class FooTheme extends Bartheme {
      public function initEngine() {
        parent::initEngine();
        $this->themeEngine()->init($this);
      }
    }
    // ...
    $theme->initEngine();
    
dawehner’s picture

FileSize
4.08 KB
19.75 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Thank you for the review!!

You're going to hate me but don't we have to use American spellings? Initialization.

Yeah the australian way just looks wrong :p

Can you elaborate on the expected format of these objects?

Reworked the documentation somehow, what do you think about it? Does @see will allow enough to be used?

General comment: boy the theme system could do with real objects and some interfaces (not related to this patch).

Your code does not really makes things clear. Why should a theme object be responsible for initialization itself, though maybe the theme engine certainly makes sense.

Status: Needs review » Needs work

The last submitted patch, 12: theme_init-2167635.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
19.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_init-2167635_1.patch. Unable to apply patch. See the log in the details link for more information. View

Ups.

dawehner’s picture

14: theme_init-2167635.patch queued for re-testing.

dawehner’s picture

14: theme_init-2167635.patch queued for re-testing.

dawehner’s picture

14: theme_init-2167635.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 14: theme_init-2167635.patch, failed testing.

sun’s picture

Hm. Some methods of the new class do not really belong to its responsibilities, IMHO.

For consistency with ModuleHandler, at least the loading of themes belongs into ThemeHandler::load(), no?

For the initialization methods, I actually wonder whether a standalone service is the right architectural tool? — Technically, the theme needs to be (re-)initialized for every Request, and ideally only on-demand upon the first call into _theme() & Co.

Lastly, the addition of theme assets, shouldn't that actually be a Response event listener, limited to HTML responses?

I think we need to rethink the proposed approach here.

dawehner’s picture

For the initialization methods, I actually wonder whether a standalone service is the right architectural tool? — Technically, the theme needs to be (re-)initialized for every Request

My thoughts back in the days have been that it might be an somehow internal service only. You as a developer should just interact with the theme negotiator or some other wrapping
service. This service is just a nice separation of concerns.

, and ideally only on-demand upon the first call into _theme() & Co.

This works theoretically if we remove the support to alter stuff in themes.

Lastly, the addition of theme assets, shouldn't that actually be a Response event listener, limited to HTML responses?

Yeah probably a kernel view subscriber, at which state we do have the fragments on which we can attach those assets.

sun’s picture

This service is just a nice separation of concerns.

Yes, there is a separation of concerns by introducing a separate service, but we're running into the exact same + major trap as with the current_user service here:

The theme depends on the current request.

How do we ensure that the correct theme is (re-)initialized for a particular request?

In addition, the D8 [contrib] port of the removed core feature of user-specific themes enhances/replaces that dependency with a dependency on the current_user service.

This works theoretically if we remove the support to alter stuff in themes.

That is definitely fine — the one and only reason for why drupal_alter() includes themes is to allow themes to participate in theme related hooks.

Any alter hook that is invoked before the theme layer is initialized is actually not expected to involve themes. The only reason for why themes participate currently is "KISS" — it simply would have been too much extra code/logic in a too frequently called code path to check whether themes are eligible to participate in a certain alter hook.

Various people actually raised major concerns against the (current) proposed solution of simply including themes in all alter hooks, because that apparently allows themes to even participate in hook_query_alter() and similarly scary alter hooks that you definitely do not want to see in any theme. ;-)

Yeah probably a kernel view subscriber, at which state we do have the fragments on which we can attach those assets.

That sounds much more adequate!

dawehner’s picture

Yes, there is a separation of concerns by introducing a separate service, but we're running into the exact same + major trap as with the current_user service here:

On the longrun I would like to get rid of global $theme, $theme_key etc. and replace all those instances with a call to our theme negotiator service, see #2167619: Replace many of the global $theme/$theme_key with a call to the theme negotiator.
This kind of service would then ensure that the theme is actually initialized by asking the theme init. service. Maybe some other wrapping of the two services would be possible as well.
One important point is that we should be able to figure out whether the theme system is initialized because you might not want to boot it up if not needed, see the module handler as example.
Would that kind of architecture work for you?

What do you exactly refer to with the problem of the current_user service. Is this about a too early init, or about not being able to change the current theme?

How do we ensure that the correct theme is (re-)initialized for a particular request?

If you have a new request, whether it is a subrequest or an actual one, the theme negotiator would not know yet about an active theme and initialize it.

Yeah probably a kernel view subscriber, at which state we do have the fragments on which we can attach those assets.

That sounds much more adequate!

Does it make sense to put that onto the assets of a HtmlPage object? Are html pages the only place which renders html with additional css?
Maybe not, as AJAX requests also add css, if needed.

dawehner’s picture

Status: Needs work » Needs review
FileSize
23.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,452 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 23: theme_init-2167635-23.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

23: theme_init-2167635-23.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: theme_init-2167635-23.patch, failed testing.

sun’s picture

I wasn't sure on which of both issues to comment, so #2167619-17: Replace many of the global $theme/$theme_key with a call to the theme negotiator.

Perhaps, shall we create a new theme system meta issue? Apparently, there are a lot of fundamental architectural questions to figure out, and I'd rather like to do that before further time is spent on patches.

sun’s picture

dawehner’s picture

This issue will be part of #2228093: Modernize theme initialization so this issue should probably be closed.

dawehner’s picture

Status: Needs work » Fixed

The other issue takes care of it.

Status: Fixed » Closed (fixed)

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