suggested commit message:
Issue #2329795 by fietserwin, bzitzow, alexrayu, tstoeckler | illutek, dawehner: Fixed Make it possible to add information to theme info files during development.
Since the other issue had first (but partial fix) merged the users involved.

Problem/Motivation

All the information stored in $theme.info.yml files is not changeable at all once a theme is enabled/installed. We need to ensure that we somehow store the two states: system.theme.data and the active theme object.

Proposed resolution

use addTheme()

Remaining tasks

  • (done) tests added
  • (done) review

User interface changes

No.

API changes

CommentFileSizeAuthor
#8 2329795-6-theme-state.patch6.97 KBtstoeckler
#5 2329795-6-theme-state-pass.patch10.86 KBtstoeckler
#5 2329795-6-theme-state-fail.patch10.86 KBtstoeckler
#2 2329795-1-theme-state.patch908 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
drush ev "\Drupal::state()->delete('theme.active_theme.ef8_theme'); \Drupal::service('theme_handler')->refreshInfo();"

You need for example the following snipped.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Active » Needs review
FileSize
2329795-1-theme-state.patch908 bytes

With this patch I can add stuff like libraries to my theme run drush cr and have them be detected by the system.

If this gets in, it definitely needs to be mentioned on https://www.drupal.org/drupal-8.0 : Drupal 8 will allow you to add JavaScript and CSS from your theme!!! I wish I could have that feature when working on D7 sites right now. Oh wait...............

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

To add some context for those following along at home:

When you enable a theme, the contents of its info file are cached in state and read from there on every request. The bad part is that this state entry is never ever cleared or re-built in any way. That means that you can add JS/CSS/libraries to your theme and put them in your info file, but they will never ever be loaded, because they are not in the state entry.

The sucker that is responsible for this is ThemeInitialization::getActiveThemeByName(). As you can see, as soon as the state entry exists it always early returns and never re-builds it. This function is also the only place in core that state entry is referenced at all.

Surely it would be nice to put the clearing of this state entry somewhere more semantic that drupal_ "Sledgehammer" _flush_all_caches(), but this at least works and gets the critical out of the way. I briefly discussed this with @dawehner and there's really no sane place (else) to put it. If we want things to be nice and shiny we probably have to refactor the theme system a fair bit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Needs tests

Is there any proper way to test this? You would have to alter the theme.info.yml file, but I can't see how we can actually add modules in the files directory? Do we have to trust us here?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue tags: -Needs tests
FileSize
2329795-6-theme-state-fail.patch10.86 KB
2329795-6-theme-state-pass.patch10.86 KB

Sure, here we go.

Some notes:
- I had to change DefaultNegotiator (the event listener that determines the active theme by asking the default theme stored in config) to inject the config factory instead of the config object directly, otherwise it will not react to changes of the active theme in the same request.
- I fixed a really nasty bug in ThemeHandler::refreshInfo() where it was writing to $this->list directly instead of using $this->addTheme(). That doesn't properly set the $theme->libraries, etc. properties properly, so that the library added for the test still wasn't being found even though it was in $theme->info['libraries']. This doesn't show up as a bug on actual sites, because it only affects the rest of the request after refreshInfo() was called. On the next request the themes will be added properly with addTheme(). (Yes, the fact that we have both $theme->info['libraries'] and $theme->libraries and they are synced only at one very specific place in code is insane (as is also mentioned in the code), but that's not for this issue to fix)

The last submitted patch, 5: 2329795-6-theme-state-fail.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/core/lib/Drupal/Core/Theme/DefaultNegotiator.php
@@ -18,9 +18,9 @@ class DefaultNegotiator implements ThemeNegotiatorInterface {
+   * @var \Drupal\Core\Config\ConfigFactoryInterface
    */
-  protected $config;
+  protected $configFactory;
 
   /**
    * Constructs a DefaultNegotiator object.
@@ -29,7 +29,7 @@ class DefaultNegotiator implements ThemeNegotiatorInterface {

@@ -29,7 +29,7 @@ class DefaultNegotiator implements ThemeNegotiatorInterface {
    *   The config factory.
    */
   public function __construct(ConfigFactoryInterface $config_factory) {
-    $this->config = $config_factory->get('system.theme');
+    $this->configFactory = $config_factory;
   }
 
   /**
@@ -43,7 +43,7 @@ public function applies(RouteMatchInterface $route_match) {

@@ -43,7 +43,7 @@ public function applies(RouteMatchInterface $route_match) {
    * {@inheritdoc}
    */
   public function determineActiveTheme(RouteMatchInterface $route_match) {
-    return $this->config->get('default');
+    return $this->configFactory->get('system.theme')->get('default');
   }

    Yeah never execute anything in the constructor phase, it is simple as that. 

  2. +++ b/core/modules/system/src/Tests/Theme/ThemeInfoTest.php
@@ -0,0 +1,111 @@
+
+    // @see theme_test_system_info_alter()
+    $this->state->set('theme_test.modify_info_files', TRUE);
+    drupal_flush_all_caches();
+    $active_theme = $this->themeManager->getActiveTheme();
+    $this->assertEqual(['core/backbone'], $active_theme->getLibraries());
+  }

    Nice!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
2329795-6-theme-state.patch6.97 KB

Ahh, just realized that I forgot "-C -M" in the diff, sorry for that. Here's the exact same patch with that.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

Awesome, it sounds like this is a proper fix + test for #2273769: Theme .info.yml file changes are not picked up even after clearing cache or running rebuild.php!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Ahh, thanks for referencing that, I wasn't aware of that issue.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

added a suggested commit message to summary in case committer wants to merge the people involved in the two issues.
(also gave the patch a read and it looks ok to me)

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

  • catch committed 57fc261 on 8.0.x 
    Issue #2329795 by fietserwin, bzitzow, alexrayu, tstoeckler | illutek,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

So this doesn't feel like the ideal fix, but #2235901: Remove custom theme settings from *.info.yml is also going to touch this code and getting the critical bug fixed with test coverage should help with the refactor.

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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