CommentFileSizeAuthor
#133 theme_registry-1886448.patch71.13 KBdawehner
#133 interdiff.txt8.48 KBdawehner
#127 interdiff.txt11.37 KBdawehner
#127 theme_registry-1886448.patch67.21 KBdawehner
#125 theme_registry-1886448-125.patch69.82 KBdawehner
#125 interdiff.txt948 bytesdawehner
#123 interdiff.txt12.76 KBdawehner
#123 theme_registry-1886448-123.patch69.39 KBdawehner
#119 theme_registry-1886448-119.patch63.39 KBdawehner
#109 theme-registry-1886448-109.patch137.74 KBBerdir
#105 theme-registry-1886448-105.patch137.7 KBBerdir
#105 theme-registry-1886448-105-interdiff.txt1.57 KBBerdir
#103 theme-registry-1886448-103.patch137.51 KBBerdir
#103 theme-registry-1886448-103-interdiff.txt522 bytesBerdir
#101 theme-registry-1886448-101.patch137.47 KBBerdir
#101 theme-registry-1886448-101-interdiff.txt16.33 KBBerdir
#94 drupal-1886448-94.patch134.32 KBdawehner
#91 drupal-1886448-91.patch135.56 KBdawehner
#86 drupal-1886448-86-mega-debugging.patch135.46 KBdawehner
#86 interdiff.txt428 bytesdawehner
#85 drupal-1886448-85.patch134.88 KBdawehner
#81 drupal-1886448-81.patch133.73 KBParisLiakos
#81 interdiff.txt850 bytesParisLiakos
#77 drupal-1886448-77.patch134.55 KBdawehner
#77 interdiff.txt3.09 KBdawehner
#76 drupal-1886448-76.patch134.37 KBdawehner
#76 interdiff.txt1.38 KBdawehner
#75 interdiff.txt2.08 KBdawehner
#73 drupal-1886448-73.patch132.99 KBdawehner
#70 drupal-1886448-68.patch133.13 KBdawehner
#66 drupal-1886448-66.patch104.56 KBdawehner
#65 drupal-1886448-65.patch133.43 KBdawehner
#65 interdiff.txt10.07 KBdawehner
#62 drupal-1886448-62.patch129.09 KBdawehner
#61 drupal-1886448-61.patch89.59 KBdawehner
#56 drupal-1886448-56.patch129.08 KBdawehner
#54 drupal-1886448-54.patch129.35 KBdawehner
#51 drupal-1886448-51.patch128.53 KBdawehner
#51 interdiff.txt4.05 KBdawehner
#44 drupal-1886448-44.patch127.34 KBdawehner
#44 interdiff.txt380 bytesdawehner
#42 drupal-1886448-42.patch126.9 KBdawehner
#38 drupal-1886448-38.patch127.12 KBdawehner
#38 interdiff.txt3.75 KBdawehner
#36 drupal-1886448-36.patch125.93 KBdawehner
#36 interdiff.txt953 bytesdawehner
#32 interdiff.txt1.44 KBdawehner
#32 drupal-1886448-32.patch126.21 KBdawehner
#30 drupal-1886448-30.patch126.53 KBdawehner
#28 drupal-1886448-28.patch125.43 KBdawehner
#28 interdiff.txt5.2 KBdawehner
#26 interdiff.txt2.09 KBdawehner
#26 drupal-1886448-26.patch123.33 KBdawehner
#24 drupal-1886448-24.patch122.97 KBdawehner
#24 interdiff.txt5.76 KBdawehner
#23 drupal-1886448-23.patch121.04 KBdawehner
#19 theme.registry.19.patch121.72 KBsun
#19 interdiff.txt15.03 KBsun
#17 theme.registry.16.patch120.16 KBsun
#17 interdiff.txt27.54 KBsun
#15 theme.registry.15.patch110.22 KBsun
#15 interdiff.txt40.91 KBsun
#12 theme.registry.12.patch95.46 KBsun
#12 interdiff.txt26.74 KBsun
#6 theme.registry.6.patch87.39 KBsun
#6 interdiff.txt791 bytessun
#4 theme.registry.4.patch87.13 KBsun
#4 interdiff.txt12.31 KBsun
#2 theme.registry.2.patch80.82 KBsun
#2 interdiff.txt66.02 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: Rewrite theme system/processing » Rewrite the theme registry into a proper service

Clarifying scope/title.

sun’s picture

Status: Active » Needs review
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI), +theme system cleanup
FileSize
66.02 KB
80.82 KB

Finally: Sanity.

Attached patch rewrites the whole thing. Fully works in manual testing, including installer. Let's see what the testbot says.

Started with a literal copy of the procedural functions (the base for the interdiff), followed by:

  1. Revamped theme registry processing for #939462: Specific preprocess functions for theme hook suggestions are not invoked, requiring explicit registration for preprocess and process functions.
  2. Fixed module hook suggestions of base hooks are not registered as base hooks.
  3. Removed obsolete procedural code, debugging code, and vastly improved documentation.
  4. Relocated load() and save() methods, renamed process() to processExtension().

Maintained in the theme-registry-1886448-sun branch of the Platform sandbox.

Status: Needs review » Needs work

The last submitted patch, theme.registry.2.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
12.31 KB
87.13 KB

Semi-final magic:

Register and inject theme registry as a proper kernel service.

Status: Needs review » Needs work

The last submitted patch, theme.registry.4.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
791 bytes
87.39 KB

D'oh, sorry. Forgot to update the installer accordingly.

Status: Needs review » Needs work
Issue tags: -theme system cleanup

The last submitted patch, theme.registry.6.patch, failed testing.

dawehner’s picture

fubhy’s picture

Revamped theme registry processing for #939462: Specific preprocess functions for theme hook suggestions are not invoked, requiring explicit registration for preprocess and process functions.

Shouldn't we do that in a follow-up? (Not that I don't agree... The magic we currently use for that is extremely fragile.)

+++ b/core/lib/Drupal/Core/Theme/Registry.phpundefined
@@ -0,0 +1,676 @@
+    if ($complete) {
+      if (!isset($this->registry)) {
+        $this->registry = $this->load();
+      }
+      return $this->registry;

Should be $this->registry = $this->load($complete); in this case... However I think it might even be better to remove $complete alltogether. Either you invoke getRuntime or you invoke get. No need for this cross-mapping.

+++ b/core/lib/Drupal/Core/Theme/Registry.phpundefined
@@ -0,0 +1,676 @@
+          }
+          // Find the preferred theme engine for this module template.
+          // @todo Remove and make Twig the default engine.
+          if ($type == 'module') {
+            $render_engines = array(
+              '.twig' => 'twig',
+              '.tpl.php' => 'phptemplate',
+            );
+            foreach ($render_engines as $extension => $engine) {
+              // Render the output using the template file.
+              $template_file = $info['path'] . '/' . $info['template'] . $extension;
+              if (file_exists($template_file)) {
+                $info['template_file'] = $template_file;
+                $info['engine'] = $engine;
+                break;
+              }
+            }
+          }
+        }
+        // Otherwise, the implementation must be a function. However, functions
+        // do not need to be specified manually; the array key of the hook is
+        // expected to be taken over as function, unless overridden.
+        elseif (!isset($info['function'])) {
+          if ($type == 'module') {
+            $info['function'] = 'theme_' . $hook;
+          }
+          else {
+            $info['function'] = $name . '_' . $hook;
+          }
+        }
+      }
+      // If no 'variables' or 'render element' was defined, then this hook
+      // definition extends an existing, or defines data for a hook suggestion.
+      // Data for hook suggestions requires a full registry in order to check
+      // for base hooks; therefore it happens after per-extension processing as
+      // last step in Registry::build().
+      else {
+        // Revert the above theme engine hack for Twig, if the actual theme
+        // engine returns a template.
+        // @todo What an insane hack is this? Simply support multiple theme engines.
+        if ($type == 'theme_engine' && isset($info['template'])) {
+          unset($registry[$hook]['template_file']);
+        }
+      }

I would go even further and remove support for multiple theme engines altogether... But that's something we should do in a separate issue. We can do that AFTER the conversion to Twig is finished.

+++ b/core/lib/Drupal/Core/Theme/Registry.phpundefined
@@ -0,0 +1,676 @@
+
+  /**
+   * Invalidates theme registry caches and rebuilds the theme registry.
+   *
+   * @return array
+   *   The complete, rebuilt theme registry.
+   */
+  public function rebuild() {
+    $this->reset();
+    return $this->get();
+  }
+

That looks rather redundant and might be abused/used incorrectly with horrible performance impact. (e.g. if a module simply want's to invalidate the theme registry but uses the API incorrectly and triggers a rebuild directly after that for no reason).

+++ b/core/lib/Drupal/Core/Theme/Registry.phpundefined
@@ -0,0 +1,676 @@
+  /**
+   * Forces the system to rebuild the theme registry.
+   *
+   * This function should be called when modules are added to the system, or when
+   * a dynamic system needs to add more theme hooks.
+   */
+  public function reset() {
+    unset($this->runtimeRegistry);
+    unset($this->registry);
+    $this->cache->invalidateTags(array('theme_registry' => TRUE));
+  }

Actually, this method just clears the theme registry and doesn't rebuild it as advertised by the docblock.

andypost’s picture

Suppose this needs benchmarks, probably overhead should not be visible

dawehner’s picture

Another possible follow-up could be to move the globals like $theme, $theme_key into a service as well.

sun’s picture

Status: Needs work » Needs review
FileSize
26.74 KB
95.46 KB
  1. Fixed base hook info is not merged into theme hook suggestion info.
  2. Removed theme_get_registry() procedural wrapper and $complete parameters.
  3. Fixed template_(pre)process(_HOOK)() functions are registered in incorrect order.
  4. Fixed missing/overlooked preprocess function theme_test.module.
  5. Allow to instantiate Theme\Registry for a particular theme.
  6. Removed Registry::rebuild().

Testbot results for this patch should show more green.

fubhy’s picture

+++ b/core/lib/Drupal/Core/Theme/Registry.phpundefined
@@ -0,0 +1,712 @@
+  public function init($theme_name = NULL) {
...
+  public function load() {
...
+  public function save($registry) {
...
+  public function build() {
...
+  public function processExtension(&$registry, $type, $name, $theme_name, $theme_path) {
...
+  protected function getHookImplementations($hook) {
...
+  public function findFunctions($cache, $prefixes) {
...
+  public function findTemplates($cache, $extension, $path) {

These should all be protected. Accessing ->get() and ->reset() should be all you really need to do from the outside.

Architecturally I find this hard to review because when looking at the (old, copied) code for the theme registry it makes me feel really uncomfortable. I guess we have to start somewhere...

Also, what are we going to do with ThemeRegistry? Let's turn the runtime theme registry into a service too so we can inject 'theme.registry' and also get rid of ->getRuntime() entirely.

Status: Needs review » Needs work

The last submitted patch, theme.registry.12.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
40.91 KB
110.22 KB

Substantial progress, incorporating most of the above feedback:

  1. Merged Registry::load() into ::get(), renamed ::save() into ::setCache(), and clarified docs.
  2. Removed unused copies of drupal_find_theme_functions() and drupal_find_theme_templates().
  3. Changed visibility for internal Registry class methods.
  4. Fixed template_(pre)process_HOOK() is not registered for themeable Views plugins.
  5. Renamed RegistryTest to RuntimeRegistryTest.
  6. Fixed missing include file declarations for theme hooks in core/includes.
  7. Fixed bogus variable names in Registry::init().
  8. Updated theme.api.php documentation. Added @todos.
  9. Added Theme\RegistryUnitTest.

The new DUTB RegistryUnitTest takes 1 second to complete, and during the entire course of this rewrite, I already thought of creating it, but got "distracted" by trying to make the patch pass all other core tests... stupid me! There's a ton of processing logic going on in the theme registry, which is completely untested right now. Now, the new RegistryUnitTest leverages the theme_test.module and test themes to verify that the contents of the registry equal our expectations (without providing actual implementations). I've added a couple of @todos there, which I'm still resolving currently.

Status: Needs review » Needs work

The last submitted patch, theme.registry.15.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
27.54 KB
120.16 KB

Fixed horizontal + vertical merging of (pre)process functions, added massive test coverage, as well as extensive docs.

Status: Needs review » Needs work

The last submitted patch, theme.registry.16.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
15.03 KB
121.72 KB
  1. Fixed drupal_find_theme_functions() + drupal_find_theme_templates() must not register variables/render element keys.
  2. Fixed (pre)process functions are not recorded in the correct order if a theme overrides a function with a template.

Essentially, I've rewritten the entire build process for (pre)process functions and introduced a compilation step that sorts them correctly.

The good news is that I'm adding massive test coverage here, so we can perfectly look into replacing the current theme registry building and compiling with a simpler implementation in follow-up issues. For now, my goals are limited and focused:

1) Turn the entire thing into a proper service.
2) Resolve the epic bug of (pre)process functions for hook suggestions.
3) Add extensive test coverage as dedicated DUTB unit test for the registry functionality only - i.e., the theme() counter-part is explicitly not involved.

Let's see what the testbot has to say about this one. :)

Status: Needs review » Needs work

The last submitted patch, theme.registry.19.patch, failed testing.

cweagans’s picture

I would go even further and remove support for multiple theme engines altogether... But that's something we should do in a separate issue. We can do that AFTER the conversion to Twig is finished.

There's already an issue open for that. I'm not sure why we'd ever want to keep that functionality, but some people seem to want it. I don't particularly care about it that much, so I haven't put much energy into arguing that point. #1537050: [meta] Should we keep / improve multiple theme engine functionality?

dawehner’s picture

@sun

How can we help to move forward on this issue? Have you made some progress, which is not yet part of that issue?

dawehner’s picture

Status: Needs work » Needs review
FileSize
121.04 KB

Let's first start with just a rerole, which doesn't fail completely and then try to fix the bugs.

dawehner’s picture

FileSize
5.76 KB
122.97 KB

Fixed some of the preprocss functions in the views_ui.module file
Injected the module handler and use it on the registry.

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
123.33 KB
2.09 KB

Fixes the scanning of twig files and a preprocess function for exposed forms in views.

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-26.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.2 KB
125.43 KB

Updates:

  • Merged changes to theme.inc since the beginning of the work of sun
  • Fixed template scanning for phptemplate
  • Fixed a bug in language module, views_test_data and tour

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
126.53 KB

Fixed a single issue in the locale.pages.inc

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-30.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
126.21 KB
1.44 KB

Now that the cache system is fully injected using a separate cache bin name but the same db table, doesn't work anymore.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

The last submitted patch, drupal-1886448-32.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#32: drupal-1886448-32.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, drupal-1886448-32.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
953 bytes
125.93 KB

Let it be installed again.

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-36.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
127.12 KB

Converted the theme registry to implement the desctructableInterface and fixes some of the tests.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

The last submitted patch, drupal-1886448-38.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#38: drupal-1886448-38.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, drupal-1886448-38.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
126.9 KB

The ThemeRegistry Object still loads the theme cache entry for itself, so we can't change that now.

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-42.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
380 bytes
127.34 KB

There seemed to be an old cache entry in there from D7, though this only is the problem for actual real life upgrade.

Sadly this doesn't fix the upgrade tests.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

The last submitted patch, drupal-1886448-44.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#44: drupal-1886448-44.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, drupal-1886448-44.patch, failed testing.

dawehner’s picture

This is insteresting: I guess we could maybe solve this with #1786490: Add caching to the state system or at least we have to figure out why a cacheArray object is created and destroyed in CoreBundle->build().

_drupal_theme_initialize(NULL, Array)
drupal_theme_initialize()
Drupal\Core\Theme\Registry->init(NULL)
Drupal\Core\Theme\Registry->__construct(Object, Object)
ReflectionClass->newInstanceArgs(Array)
Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'theme.registry')
Symfony\Component\DependencyInjection\ContainerBuilder->get('theme.registry')
Drupal\Core\Utility\ThemeRegistry->initializeRegistry()
Drupal\Core\Utility\ThemeRegistry->set(Array)
Drupal\Core\Utility\CacheArray->__destruct()
Drupal\Core\CoreBundle->build(Object)
Drupal\Core\DrupalKernel->buildContainer()
Drupal\Core\DrupalKernel->initializeContainer()
Drupal\Core\DrupalKernel->boot()
Drupal\Core\DrupalKernel->updateModules(Array, Array)
Drupal\simpletest\DrupalUnitTestBase->enableModules(Array, )
Drupal\simpletest\DrupalUnitTestBase->setUp()
Drupal\views\Tests\ViewUnitTestBase->setUp()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('703', 'Drupal\views\Tests\ViewExecutableTest')
xjm’s picture

Priority: Normal » Critical

Discussed in IRC: This is the preserved preferred way to resolve #939462: Specific preprocess functions for theme hook suggestions are not invoked, so let's make this a critical.

Berdir’s picture

You could override __destruct(), do nothing there and instead add destruct(). I did that in AliasWhitelist, I think.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.05 KB
128.53 KB

Sadly this doesn't fix the upgrade tests, maybe it helps for the ones, which doesn't fail locally.

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-51.patch, failed testing.

Berdir’s picture

+++ b/core/includes/install.core.incundefined
@@ -369,7 +369,8 @@ function install_begin_request(&$install_state) {
-      ->addArgument(new Reference('module_handler'));
+      ->addArgument(new Reference('module_handler'))
+      ->addTag('needs_destruction');

I don't think this works in the installer as it requires compiler passes and we don't compile here I think.

dawehner’s picture

Status: Needs work » Needs review
FileSize
129.35 KB

Wow, after hours of debugging at least the twig debug test got fixed.

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-54.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
129.08 KB

Fixed the notice and rerolled for the services patch.

jibran’s picture

Great Work @dawehner. Yay! patch is green now.

dawehner’s picture

dawehner’s picture

#56: drupal-1886448-56.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, drupal-1886448-56.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
89.59 KB

Rerolled against moving template.php to $theme.theme

dawehner’s picture

FileSize
129.09 KB

Forgot some files.

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-62.patch, failed testing.

ParisLiakos’s picture

+++ b/core/includes/common.incundefined
@@ -1959,7 +1959,7 @@ function l($text, $path, array $options = array()) {
+      $registry = drupal_container()->get('theme.registry')->getRuntime();

+++ b/core/includes/theme.maintenance.incundefined
@@ -88,7 +88,10 @@ function _drupal_maintenance_theme() {
+  drupal_container()->get('theme.registry');

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.phpundefined
@@ -76,7 +77,8 @@ function __construct($cid, $bin, $tags, $modules_loaded = FALSE) {
+    $this->completeRegistry = drupal_container()->get('theme.registry')->get();

@@ -111,8 +113,9 @@ public function offsetGet($offset) {
+      $this->completeRegistry = drupal_container()->get('theme.registry')->get();

+++ b/core/modules/contextual/contextual.moduleundefined
@@ -144,7 +144,7 @@ function contextual_preprocess(&$variables, $hook) {
-  $hooks = theme_get_registry(FALSE);
+  $hooks = drupal_container()->get('theme.registry')->getRuntime();

+++ b/core/modules/system/theme.api.phpundefined
@@ -120,7 +122,7 @@ function hook_preprocess(&$variables, $hook) {
+    $hooks = drupal_container()->get('theme.registry')->getRuntime();

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -1719,54 +1719,19 @@ public function buildOptionsForm(&$form, &$form_state) {
+          $cache_theme = drupal_container()->get('cache.theme');

lets use \Drupal

+++ b/core/includes/theme.incundefined
@@ -830,6 +414,13 @@ function drupal_find_base_themes($themes, $key, $used_keys = array()) {
 /**
+ * @todo Remove this. "Rebuild" is a misnomer in the first place.
+ */
+function drupal_theme_rebuild() {
+  \Drupal::service('theme.registry')->reset();
+}

Lets get rid of this
Drupal::service('theme.registry')->reset();
is a good enough one liner:)

dawehner’s picture

FileSize
10.07 KB
133.43 KB

Thanks for your review rootatwc!

Fixed all your points.

I wasn't able to fix the test yet. The problem is basically that the theme registry service is initialized too early in the
process, so it can't be switched to the new theme.

dawehner’s picture

Status: Needs work » Needs review
FileSize
104.56 KB

Rerolled the patch against some recent core changes.

Additional the test got fixed by resetting the global variables, so that the theme can call drupal_theme_initialize() again, and get the right default theme.

config('system.theme')
      ->set('default', 'test_theme_twig')
      ->save();
    theme_enable(array('test_theme_twig'));
    // Unset the global variables, so \Drupal\Core\Theme::init() fires
    // drupal_theme_initialize, which fills up the global variables properly and
    // choosed the current active theme.
    unset($GLOBALS['theme_info']);
    unset($GLOBALS['theme']);
    // Reset the theme registry, so that the new theme is used.
    $this->container->set('theme.registry', NULL);

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

The last submitted patch, drupal-1886448-66.patch, failed testing.

bcn’s picture

Status: Needs work » Needs review

#66: drupal-1886448-66.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, drupal-1886448-66.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
133.13 KB

Forgot the Theme Registry file :(

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-68.patch, failed testing.

ParisLiakos’s picture

+++ b/core/lib/Drupal/Core/Theme/Registry.phpundefined
@@ -0,0 +1,718 @@
+  protected $base_themes;

should be baseThemes

dawehner’s picture

Status: Needs work » Needs review
FileSize
132.99 KB

This is green again!

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-73.patch, failed testing.

dawehner’s picture

FileSize
2.08 KB

Posting the interdiff of the previous comment.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
134.37 KB
dawehner’s picture

FileSize
3.09 KB
134.55 KB

Fixed some comments + the review from #72

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

The last submitted patch, drupal-1886448-77.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#77: drupal-1886448-77.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, drupal-1886448-77.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
850 bytes
133.73 KB

this edit fail..is weird..the array order changes.
lets see this green again..and then decide what to do with all these todo's

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

The last submitted patch, drupal-1886448-81.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#81: drupal-1886448-81.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, drupal-1886448-81.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
134.88 KB

I know this still fails, but this is just a rerole.

dawehner’s picture

Wow, I needed more then a week for this line ...

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-86-mega-debugging.patch, failed testing.

dawehner’s picture

There might be also an issue on module_enable, see http://drupal.org/node/1975354#comment-7398450

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, drupal-1886448-86-mega-debugging.patch, failed testing.

dawehner’s picture

Assigned: sun » Unassigned
Status: Needs work » Needs review
FileSize
135.56 KB

Just a rerole + potential fix, at least all the theme and image tests run fine.

tim.plunkett’s picture

I think the @todo's for using $this->registry throughout should be resolved before commit, but the rest are mostly just baggage from theme.inc.

+++ b/core/includes/theme.maintenance.incundefined
@@ -90,7 +90,10 @@ function _drupal_maintenance_theme() {
+  // Prime the theme registry.
+  // @todo Remove global theme variables.
+  Drupal::service('theme.registry');

This worries me, since there is work being done to make services lazy-load. Then this would stop doing anything.

+++ b/core/lib/Drupal/Core/Theme/Registry.phpundefined
@@ -0,0 +1,717 @@
+   * @global object $theme_info

I've not seen @global before, but dawehner says it is standard phpdoc

+++ b/core/modules/help/help.moduleundefined
@@ -66,6 +66,16 @@ function help_help($path, $arg) {
 /**
+ * Implements hook_theme().
+ */
+function help_theme() {
+  $theme['block__system_help_block'] = array(
+    'preprocess' => array('help_preprocess_block'),
+  );
+  return $theme;

This is going to be a huge PITA, but it does help resolve ambiguity...

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/RegistryTest.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\system\Tests\Theme\RegistryTest.
+ * Definition of Drupal\system\Tests\Theme\RuntimeRegistryTest.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/RegistryUnitTest.phpundefined
@@ -0,0 +1,294 @@
+ * Definition of Drupal\system\Tests\Theme\RegistryUnitTest.

Contains

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/RegistryUnitTest.phpundefined
@@ -0,0 +1,294 @@
+  protected $registries = array();

Needs a docblock

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/RegistryUnitTest.phpundefined
@@ -0,0 +1,294 @@
+  function setUp() {
...
+  function xtestThemeScope() {

All of these need visibility public/protected

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/RegistryUnitTest.phpundefined
@@ -0,0 +1,294 @@
+   * Overrides TestBase::assertEqual().
...
+   * @see http://drupal.org/node/1601146
+   */
+  protected function assertEqual($actual, $expected, $message = NULL, $group = 'Other') {
...
+  protected function assertIsset($array, $key, $message = NULL, $args = array()) {
...
+  protected function assertNotIsset($array, $key, $message = NULL, $args = array()) {
...
+  protected function assertInArray($array, $value, $message = NULL, $args = array()) {

This seems like something sun shoved in... Oh well.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTestTwig.phpundefined
@@ -37,7 +37,7 @@ function setUp() {
-  function testTemplateOverride() {
+  function _testTemplateOverride() {

@@ -48,14 +48,8 @@ function testTemplateOverride() {
-  function testFindThemeTemplates() {
...
+  function _testFindThemeTemplates() {

Not okay :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -1725,54 +1725,19 @@ public function buildOptionsForm(&$form, &$form_state) {
         if (isset($GLOBALS['theme']) && $GLOBALS['theme'] == $this->theme) {
-          $this->theme_registry = theme_get_registry();
+          $this->theme_registry = \Drupal::service('theme.registry')->get();
           $theme_engine = $GLOBALS['theme_engine'];
         }
         else {
           $themes = list_themes();
           $theme = $themes[$this->theme];
-
-          // Find all our ancestor themes and put them in an array.
-          $base_theme = array();
-          $ancestor = $this->theme;
-          while ($ancestor && isset($themes[$ancestor]->base_theme)) {
-            $ancestor = $themes[$ancestor]->base_theme;
-            $base_theme[] = $themes[$ancestor];
-          }
-
-          // The base themes should be initialized in the right order.
-          $base_theme = array_reverse($base_theme);
-
-          // This code is copied directly from _drupal_theme_initialize()
+          // @see _drupal_theme_initialize()
           $theme_engine = NULL;
-
-          // Initialize the theme.
           if (isset($theme->engine)) {
-            // Include the engine.
-            include_once DRUPAL_ROOT . '/' . $theme->owner;
-
             $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);
-            }
-          }
-          else {
-            // include non-engine theme files
-            foreach ($base_theme as $base) {
-              // Include the theme file or the engine.
-              if (!empty($base->owner)) {
-                include_once DRUPAL_ROOT . '/' . $base->owner;
-              }
-            }
-            // and our theme gets one too.
-            if (!empty($theme->owner)) {
-              include_once DRUPAL_ROOT . '/' . $theme->owner;
-            }
           }
-          $this->theme_registry = _theme_load_registry($theme, $base_theme, $theme_engine);
+          $cache_theme = drupal_container()->get('cache.theme');
+          $this->theme_registry = new Registry($cache_theme, $theme->name);

Beautiful!

+++ b/core/themes/bartik/bartik.themeundefined
@@ -6,6 +6,24 @@
 /**
+ * Implements hook_theme().
+ */
+function bartik_theme() {
+  $theme['html'] = array(
+    'preprocess' => array('bartik_preprocess_html'),
+    'process' => array('bartik_process_html'),
+  );
+  $theme['maintenance_page'] = array(
+    'preprocess' => array('bartik_preprocess_maintenance_page'),
+    'process' => array('bartik_process_maintenance_page'),
+  );
+  $theme['page'] = array(
+    'process' => array('bartik_process_page'),
+  );
+  return $theme;

Even themes have to specify these? :(

barraponto’s picture

Yeah, as a themer, I think declaring preprocess functions hurts my DX. Unless my base theme does autodetection magic (but it would be a regression, anyway).

dawehner’s picture

FileSize
134.32 KB

This worries me, since there is work being done to make services lazy-load. Then this would stop doing anything.

It feels okay to accept this for the maintenance theme, as it's not on the actual site.

I've not seen @global before, but dawehner says it is standard phpdoc

See http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...

Even themes have to specify these? :(

As far as I understood #939462-170: Specific preprocess functions for theme hook suggestions are not invoked right it feels okay, but yeah we need a discussion whether we want to go with this route.

Status: Needs review » Needs work

The last submitted patch, drupal-1886448-94.patch, failed testing.

Fabianx’s picture

Twig Initiative here, we don't like to specify preprocess.

As you are rewriting this anyway: This is how we would like our theme system to work:

- Still needs a registry, but really only for registering hooks and finding templates - DONE.

No more magic, very very similar to FORM API.

Example for terminology:

- hook = hook
- themeID = node
- suggestionID = article

1.

hook_theme - as is

2.

home_theme_suggestions_alter
home_theme_suggestions_themeID_alter

The only way to add or remove suggestions. Replaces $variables['#theme_suggestions']. Also allows altering the default found suggestion, similar to the display_id in views_preprocess_view.

3.

hook_theme_prepare
hook_theme_prepare_themeID

Prepares variables for output in a theme (like a link moving from path, text to url, check_plained text), replaces template_preprocess*

4.

hook_theme_prepare_alter
hook_theme_prepare_themeID_alter
hook_theme_prepare_themeID_suggestionID_alter

Allows altering the variables, like form_formID alter for the suggestionID.

5. Output

- theme function -> HTML
- template -> HTML
- special render = TRUE / FALSE flag as third parameter to return variables as is -> array().

And thats it.

Clean, extensible, no magic, and works exactly like form API.

and _alter is fast by definition and the right thing to do here.

fubhy’s picture

#96 this... +1000 totally agreed

barraponto’s picture

hook_theme_suggestions_alter rocks.

c4rl’s picture

With regard to #96 and the general progress here, I'm adding #1899454: [meta] Refactor Render API as a related issue to the summary. There's some potential overlap with both of these.

Since I initially filed that issue I've been preoccupied with the Twig conversion, but now that Twig has gained some solid momentum, I will be diving in to Render and theme-system related issues full speed.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

jenlampton’s picture

Issue summary: View changes

rewrite with some context :)

jenlampton’s picture

Issue summary: View changes

added back C4rl's render api link

quicksketch’s picture

Issue summary: View changes

Copy of the revision from May 26, 2013 - 15:46.

jenlampton’s picture

After further consideration, I think maybe making the theme system work how we want it to work, and turning it into a proper service may be two separate issues. I've created #2004872: [meta] Theme system architecture changes.

If this is a mistake and you'd prefer to handle all the new hooks here as well, than feel free to close that issue as a dupe of this one, and update this summary to include those changes as well. (I'd originally done that, but thought better of it)

Berdir’s picture

Status: Needs work » Needs review
FileSize
16.33 KB
137.47 KB

Re-roll and changed ThemeRegistry to use a CacheCollector, see interdiff.

Crazy amount of conflicts, let's see how this goes.

Status: Needs review » Needs work

The last submitted patch, theme-registry-1886448-101.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
522 bytes
137.51 KB

Ups. This might fix the installer.

Status: Needs review » Needs work

The last submitted patch, theme-registry-1886448-103.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
137.7 KB

Fixed a merge conflict in user_theme(), a typo in ThemeRegistry and added a missing preprocess definition, there are probably more of those.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

The last submitted patch, theme-registry-1886448-105.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#105: theme-registry-1886448-105.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, theme-registry-1886448-105.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
137.74 KB

Lost the interdiff from #103.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Dependency Injection (DI)

The last submitted patch, theme-registry-1886448-109.patch, failed testing.

fgm’s picture

Status: Needs work » Needs review

#109: theme-registry-1886448-109.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Dependency Injection (DI)

The last submitted patch, theme-registry-1886448-109.patch, failed testing.

jenlampton’s picture

Priority: Critical » Normal

Is this really critical? I don't think it's blocking anything we're working on since we have new ways to solve #939462: Specific preprocess functions for theme hook suggestions are not invoked
Bumping to normal.

dawehner’s picture

This patch potentially allows to enable lazyloading, so things like rest requests can serve the data faster.

catch’s picture

The theme registry is already loaded on demand (i.e. via the first theme() call, not when the theme system is initialized). I don't think this is a release blocker, but it's also unlikely to cause any public API changes (or no more than CacheArray did in Drupal 7), so assuming that's the case it could get in after this week once it's ready.

Crell’s picture

Tagging. This would be good for performance to not always initialize the theme system, but would be an API change at this point. Will need a decision from on-high to bump to major, after there's a good issue summary.

moshe weitzman’s picture

@Crell - read the comment before yours. We already lazy load the registry. Are you suggesting that we want to avoid theme init too?

andypost’s picture

andypost’s picture

Issue summary: View changes

Reverted to the previous revision, and added back a link to c4rl's render API issue

dawehner’s picture

Issue summary: View changes
Issue tags: +PHPUnit
FileSize
63.39 KB

Decided to give it another try and started with a new patch which tries to change no existing behavior.

Instead of using a DrupalUnitTest it also mocks everything in a proper unit test.

dawehner’s picture

Status: Needs work » Needs review
jibran’s picture

  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -0,0 +1,593 @@
    +    // serve as the basic registry. Since the list of enabled modules is the same
    ...
    +   *   array keyed by theme hooks, whose values are associative arrays describing
    ...
    +   *     name is used. The default theme function name is the theme hook prefixed
    ...
    +   *     theme hook. The template is in the directory defined by the 'path' key of
    ...
    +   *   'base_theme'. Unlike regular hooks that can only be implemented by modules,
    ...
    +   *   example, if a theme hook is both defined by a module and a theme, then the
    ...
    +        // $result[$hook] will only contain key/value pairs for information being
    +        // overridden.  Pull the rest of the information from what was defined by
    

    80 chars

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -0,0 +1,593 @@
    +  protected function getPath($module) {
    ...
    +  protected function listThemes() {
    ...
    +  protected function initializeTheme() {
    

    doc missing.

  3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -0,0 +1,593 @@
    +} ¶
    

    White space.

  4. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
    @@ -8,16 +8,19 @@
    + * A cache collector to allow the theme registry to be accessed as a
    ...
      * that are actually in use on the site. On cache misses the complete
    

    expand to 80 chars.

  5. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
    @@ -0,0 +1,140 @@
    +    include_once DRUPAL_ROOT . '/core/modules/system/tests/modules/theme_test/theme_test.module';
    

    oops!!!!

Status: Needs review » Needs work

The last submitted patch, 119: theme_registry-1886448-119.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 119: theme_registry-1886448-119.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
69.39 KB
12.76 KB

This could be green now.

Status: Needs review » Needs work

The last submitted patch, 123: theme_registry-1886448-123.patch, failed testing.

dawehner’s picture

Fixed the last failure.

amateescu’s picture

  1. +++ b/core/includes/theme.inc
    @@ -912,18 +575,19 @@ function theme($hook, $variables = array()) {
    +  $hooks = \Drupal::service('theme.registry')->getRuntime();
    ...
    +      if ($hooks->has($candidate)) {
    
    @@ -935,16 +599,16 @@ function theme($hook, $variables = array()) {
    +  if (!$hooks->has($hook)) {
    

    Since we're touching those lines already, can we please rename the variable to $theme_registry? $hooks->has($hook) doesn't make any sense :)

  2. +++ b/core/includes/update.inc
    @@ -458,6 +458,9 @@ function update_prepare_d8_bootstrap() {
    +  // Remove the theme_registry cache entry from D7.
    +  Drupal::cache('cache')->deleteAll();
    

    You're removing much more than just theme_registry cache, I think the comment needs to be changed..

  3. +++ b/core/includes/update.inc
    index 9e3a6a1..d43f83e 100644
    --- a/core/lib/Drupal/Core/Path/AliasWhitelist.php
    
    --- a/core/lib/Drupal/Core/Path/AliasWhitelist.php
    +++ b/core/lib/Drupal/Core/Path/AliasWhitelist.php
    

    Unrelated change?

  4. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -0,0 +1,599 @@
    +      $this->registry = $this->build($this->theme, $this->baseThemes, $this->engine);
    

    This looks a bit weird now because the build() method already has access to those arguments.

  5. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
    index 2003e4e..54b3495 100644
    --- a/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
    
    --- a/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
    +++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
    

    Another unrelated change?

  6. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.php
    @@ -82,18 +82,24 @@ function testTwigCacheOverride() {
    +    // Unset the global variables, so \Drupal\Core\Theme::init() fires
    

    You mean \Drupal\Core\Theme\Registry::init()?

  7. +++ b/core/modules/system/system.api.php
    @@ -1348,7 +1348,7 @@ function hook_theme($existing, $type, $theme, $path) {
    + * added by \Drupal\Core\Theme\Registry::_theme_process_registry().
    

    ::processExtension() :)

  8. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
    @@ -0,0 +1,141 @@
    +   * The mocked lock backend..
    

    Double period.

dawehner’s picture

FileSize
67.21 KB
11.37 KB

Fixed the points.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

The interdiff looks nice, so assuming the bot comes back green, I think it's ready.

Xano’s picture

127: theme_registry-1886448.patch queued for re-testing.

Xano’s picture

127: theme_registry-1886448.patch queued for re-testing.

alexpott’s picture

  1. +++ b/core/includes/theme.inc
    @@ -299,354 +293,23 @@ function _drupal_theme_initialize($theme, $base_theme = array(), $registry_callb
    -function _theme_load_registry($theme, $base_theme = NULL, $theme_engine = NULL, $complete = TRUE) {
    

    This function is still called in DisplayPluginBase::displayOptionsForm()

    $this->theme_registry = _theme_load_registry($theme, $base_theme, $theme_engine);
    

    So either there are no tests for this or it is dead code!

  2. +++ b/core/includes/theme.inc
    @@ -299,354 +293,23 @@ function _drupal_theme_initialize($theme, $base_theme = array(), $registry_callb
    -      cache()->set("theme_registry:build:modules", $cache, CacheBackendInterface::CACHE_PERMANENT, array('theme_registry' => TRUE));
    

    The use Drupal\Core\Cache\CacheBackendInterface; can be removed from the top of the file. And also we can remove use Drupal\Core\Utility\ThemeRegistry; too.

  3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -0,0 +1,599 @@
    +   * @param string $theme
    +   *   The loaded $theme object as returned by list_themes().
    +   * @param array $base_theme
    +   *   An array of loaded $theme objects representing the ancestor themes in
    +   *   oldest first order.
    +   * @param string $theme_engine
    +   *   The name of the theme engine.
    

    build() no longer takes any params

  4. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -0,0 +1,599 @@
    +   *   _theme_process_registry() is called in aforementioned order and new
    +   *   entries override older ones. For example, if a theme hook is both defined
    +   *   by a module and a theme, then the definition in the theme will be used.
    

    _theme_process_registry() has been removed by this patch - in fact here it should be changed to be the method that this docblock is documenting :)

  5. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -0,0 +1,599 @@
    +  /**
    +   * Wraps drupal_get_path().
    +   *
    +   * @return string
    +   */
    +  protected function getPath($module) {
    +    return drupal_get_path('module', $module);
    

    Missing @param

  6. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -0,0 +1,599 @@
    +    return drupal_theme_initialize();
    

    drupal_theme_initialize() does not return anything.

  7. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
    @@ -38,22 +41,28 @@ class ThemeRegistry extends CacheArray {
           $data = $cached->data;
    

    $data is no longer used so this line can be removed.

  8. +++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/EventSubscriber/ThemeTestSubscriber.php
    @@ -7,6 +7,9 @@
    +use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
    ...
    +use Symfony\Component\DependencyInjection\IntrospectableContainerInterface;
    

    Not necessary.

  9. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
    @@ -0,0 +1,150 @@
    +   * @var \Drupal\Core\Theme\Registry
    

    I think this should just be @var TestRegistry since this variable is not an instance of \Drupal\Core\Theme\Registry

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#131 means that this needs work :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.48 KB
71.13 KB

Great review, thank you!

This function is still called in DisplayPluginBase::displayOptionsForm()

Damn right, this was part of the last version before the retry.

The only issue I can think of at the moment is https://drupal.org/node/2049209#comment-7993037

The use Drupal\Core\Cache\CacheBackendInterface; can be removed from the top of the file. And also we can remove use Drupal\Core\Utility\ThemeRegistry; too.

Good catch.

build() no longer takes any params

Removed, thanks.

_theme_process_registry() has been removed by this patch - in fact here it should be changed to be the method that this docblock is documenting :)

We can just point to ourselves.

drupal_theme_initialize() does not return anything.

You seem to use storm for review as well.

I think this should just be @var TestRegistry since this variable is not an instance of \Drupal\Core\Theme\Registry

Fixed as well.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Ooh, I like the changes to Views in that last interdiff.
I think that adequately addresses @alexpott's questions.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 133: theme_registry-1886448.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

133: theme_registry-1886448.patch queued for re-testing.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, the changes to Views look great! Thanks for all your work on this @dawehner and thanks to @alexpott and @amateescu for the detailed reviews.

Back to RTBC per #134 because the patch is green.

sun’s picture

Sad to see the originally contained improvements gone, but I understand that I was probably shooting for too much at once.

Thanks a ton to @dawehner and @Berdir for taking this on and getting the main change done! :)

alexpott’s picture

Title: Rewrite the theme registry into a proper service » Change notice: Rewrite the theme registry into a proper service
Status: Reviewed & tested by the community » Active
Issue tags: -Needs issue summary update +Needs change record

Committed 3cda830 and pushed to 8.x. Thanks!

dawehner’s picture

Status: Active » Needs review
markhalliwell’s picture

Title: Change notice: Rewrite the theme registry into a proper service » Rewrite the theme registry into a proper service
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks good! Made a few changes to verbiage though.

dawehner’s picture

Tthank you jibran and Marc Carver for these updates.

yched’s picture

Looks like the patch that got in still had a few @todos ?

star-szr’s picture

Issue summary: View changes

We did not go the route of manually declaring preprocess functions.

Status: Fixed » Closed (fixed)

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

larowlan’s picture

Issue tags: +theme service

retro adding the theme service tag

sun’s picture

sun’s picture