Files: 
CommentFileSizeAuthor
#133 theme_registry-1886448.patch71.13 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,046 pass(es).
[ View ]
#133 interdiff.txt8.48 KBdawehner
#127 interdiff.txt11.37 KBdawehner
#127 theme_registry-1886448.patch67.21 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,077 pass(es).
[ View ]
#125 theme_registry-1886448-125.patch69.82 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,969 pass(es).
[ View ]
#125 interdiff.txt948 bytesdawehner
#123 interdiff.txt12.76 KBdawehner
#123 theme_registry-1886448-123.patch69.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,848 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#119 theme_registry-1886448-119.patch63.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,382 pass(es), 4 fail(s), and 3 exception(s).
[ View ]
#109 theme-registry-1886448-109.patch137.74 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,389 pass(es), 102 fail(s), and 218 exception(s).
[ View ]
#105 theme-registry-1886448-105.patch137.7 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#105 theme-registry-1886448-105-interdiff.txt1.57 KBBerdir
#103 theme-registry-1886448-103.patch137.51 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 56,574 pass(es), 115 fail(s), and 7,082 exception(s).
[ View ]
#103 theme-registry-1886448-103-interdiff.txt522 bytesBerdir
#101 theme-registry-1886448-101.patch137.47 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#101 theme-registry-1886448-101-interdiff.txt16.33 KBBerdir
#94 drupal-1886448-94.patch134.32 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,942 pass(es), 5 fail(s), and 2 exception(s).
[ View ]
#91 drupal-1886448-91.patch135.56 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,914 pass(es).
[ View ]
#86 drupal-1886448-86-mega-debugging.patch135.46 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1886448-86-mega-debugging.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#86 interdiff.txt428 bytesdawehner
#85 drupal-1886448-85.patch134.88 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 53,999 pass(es), 40 fail(s), and 23 exception(s).
[ View ]
#81 drupal-1886448-81.patch133.73 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1886448-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#81 interdiff.txt850 bytesParisLiakos
#77 drupal-1886448-77.patch134.55 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,554 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#77 interdiff.txt3.09 KBdawehner
#76 drupal-1886448-76.patch134.37 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,789 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#76 interdiff.txt1.38 KBdawehner
#75 interdiff.txt2.08 KBdawehner
#73 drupal-1886448-73.patch132.99 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,509 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#70 drupal-1886448-68.patch133.13 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,916 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#66 drupal-1886448-66.patch104.56 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#65 drupal-1886448-65.patch133.43 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1886448-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#65 interdiff.txt10.07 KBdawehner
#62 drupal-1886448-62.patch129.09 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,416 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#61 drupal-1886448-61.patch89.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#56 drupal-1886448-56.patch129.08 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1886448-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#54 drupal-1886448-54.patch129.35 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 53,472 pass(es), 55 fail(s), and 88 exception(s).
[ View ]
#51 drupal-1886448-51.patch128.53 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 53,314 pass(es), 57 fail(s), and 78 exception(s).
[ View ]
#51 interdiff.txt4.05 KBdawehner
#44 drupal-1886448-44.patch127.34 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,402 pass(es), 97 fail(s), and 83 exception(s).
[ View ]
#44 interdiff.txt380 bytesdawehner
#42 drupal-1886448-42.patch126.9 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,619 pass(es), 95 fail(s), and 83 exception(s).
[ View ]
#38 drupal-1886448-38.patch127.12 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 43,316 pass(es), 3,037 fail(s), and 25,725 exception(s).
[ View ]
#38 interdiff.txt3.75 KBdawehner
#36 drupal-1886448-36.patch125.93 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,443 pass(es), 92 fail(s), and 86 exception(s).
[ View ]
#36 interdiff.txt953 bytesdawehner
#32 interdiff.txt1.44 KBdawehner
#32 drupal-1886448-32.patch126.21 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#30 drupal-1886448-30.patch126.53 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,608 pass(es), 60 fail(s), and 86 exception(s).
[ View ]
#28 drupal-1886448-28.patch125.43 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,515 pass(es), 69 fail(s), and 82 exception(s).
[ View ]
#28 interdiff.txt5.2 KBdawehner
#26 interdiff.txt2.09 KBdawehner
#26 drupal-1886448-26.patch123.33 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,705 pass(es), 130 fail(s), and 94 exception(s).
[ View ]
#24 drupal-1886448-24.patch122.97 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,610 pass(es), 151 fail(s), and 231 exception(s).
[ View ]
#24 interdiff.txt5.76 KBdawehner
#23 drupal-1886448-23.patch121.04 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,699 pass(es), 201 fail(s), and 3,273 exception(s).
[ View ]
#19 theme.registry.19.patch121.72 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,253 pass(es), 218 fail(s), and 2,763 exception(s).
[ View ]
#19 interdiff.txt15.03 KBsun
#17 theme.registry.16.patch120.16 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,301 pass(es), 243 fail(s), and 3,774 exception(s).
[ View ]
#17 interdiff.txt27.54 KBsun
#15 theme.registry.15.patch110.22 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,103 pass(es), 246 fail(s), and 2,668 exception(s).
[ View ]
#15 interdiff.txt40.91 KBsun
#12 theme.registry.12.patch95.46 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,530 pass(es), 555 fail(s), and 4,143 exception(s).
[ View ]
#12 interdiff.txt26.74 KBsun
#6 theme.registry.6.patch87.39 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,478 pass(es), 851 fail(s), and 8,891 exception(s).
[ View ]
#6 interdiff.txt791 bytessun
#4 theme.registry.4.patch87.13 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 interdiff.txt12.31 KBsun
#2 theme.registry.2.patch80.82 KBsun
FAILED: [[SimpleTest]]: [MySQL] 30,589 pass(es), 10,290 fail(s), and 10,047 exception(s).
[ View ]
#2 interdiff.txt66.02 KBsun

Comments

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

Clarifying scope/title.

Status:Active» Needs review
Issue tags:+Framework Initiative, +API clean-up, +Dependency Injection (DI), +theme system cleanup
StatusFileSize
new66.02 KB
new80.82 KB
FAILED: [[SimpleTest]]: [MySQL] 30,589 pass(es), 10,290 fail(s), and 10,047 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new12.31 KB
new87.13 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new791 bytes
new87.39 KB
FAILED: [[SimpleTest]]: [MySQL] 48,478 pass(es), 851 fail(s), and 8,891 exception(s).
[ View ]

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.

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.

Suppose this needs benchmarks, probably overhead should not be visible

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

Status:Needs work» Needs review
StatusFileSize
new26.74 KB
new95.46 KB
FAILED: [[SimpleTest]]: [MySQL] 48,530 pass(es), 555 fail(s), and 4,143 exception(s).
[ View ]
  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.

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new40.91 KB
new110.22 KB
FAILED: [[SimpleTest]]: [MySQL] 49,103 pass(es), 246 fail(s), and 2,668 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new27.54 KB
new120.16 KB
FAILED: [[SimpleTest]]: [MySQL] 49,301 pass(es), 243 fail(s), and 3,774 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new15.03 KB
new121.72 KB
FAILED: [[SimpleTest]]: [MySQL] 50,253 pass(es), 218 fail(s), and 2,763 exception(s).
[ View ]
  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.

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?

@sun

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

Status:Needs work» Needs review
StatusFileSize
new121.04 KB
FAILED: [[SimpleTest]]: [MySQL] 52,699 pass(es), 201 fail(s), and 3,273 exception(s).
[ View ]

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

StatusFileSize
new5.76 KB
new122.97 KB
FAILED: [[SimpleTest]]: [MySQL] 52,610 pass(es), 151 fail(s), and 231 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new123.33 KB
FAILED: [[SimpleTest]]: [MySQL] 52,705 pass(es), 130 fail(s), and 94 exception(s).
[ View ]
new2.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.

Status:Needs work» Needs review
StatusFileSize
new5.2 KB
new125.43 KB
FAILED: [[SimpleTest]]: [MySQL] 52,515 pass(es), 69 fail(s), and 82 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new126.53 KB
FAILED: [[SimpleTest]]: [MySQL] 52,608 pass(es), 60 fail(s), and 86 exception(s).
[ View ]

Fixed a single issue in the locale.pages.inc

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new126.21 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new1.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.

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.

Status:Needs work» Needs review
StatusFileSize
new953 bytes
new125.93 KB
FAILED: [[SimpleTest]]: [MySQL] 50,443 pass(es), 92 fail(s), and 86 exception(s).
[ View ]

Let it be installed again.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.75 KB
new127.12 KB
FAILED: [[SimpleTest]]: [MySQL] 43,316 pass(es), 3,037 fail(s), and 25,725 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new126.9 KB
FAILED: [[SimpleTest]]: [MySQL] 50,619 pass(es), 95 fail(s), and 83 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new380 bytes
new127.34 KB
FAILED: [[SimpleTest]]: [MySQL] 50,402 pass(es), 97 fail(s), and 83 exception(s).
[ View ]

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.

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.

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')

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.

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

Status:Needs work» Needs review
StatusFileSize
new4.05 KB
new128.53 KB
FAILED: [[SimpleTest]]: [MySQL] 53,314 pass(es), 57 fail(s), and 78 exception(s).
[ View ]

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.

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new129.35 KB
FAILED: [[SimpleTest]]: [MySQL] 53,472 pass(es), 55 fail(s), and 88 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new129.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1886448-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed the notice and rerolled for the services patch.

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

#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.

Status:Needs work» Needs review
StatusFileSize
new89.59 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Rerolled against moving template.php to $theme.theme

StatusFileSize
new129.09 KB
FAILED: [[SimpleTest]]: [MySQL] 54,416 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Forgot some files.

Status:Needs review» Needs work

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

+++ 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:)

StatusFileSize
new10.07 KB
new133.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1886448-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new104.56 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new133.13 KB
FAILED: [[SimpleTest]]: [MySQL] 54,916 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Forgot the Theme Registry file :(

Status:Needs review» Needs work

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

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

should be baseThemes

Status:Needs work» Needs review
StatusFileSize
new132.99 KB
FAILED: [[SimpleTest]]: [MySQL] 55,509 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

This is green again!

Status:Needs review» Needs work

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

StatusFileSize
new2.08 KB

Posting the interdiff of the previous comment.

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
new134.37 KB
FAILED: [[SimpleTest]]: [MySQL] 55,789 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

StatusFileSize
new3.09 KB
new134.55 KB
FAILED: [[SimpleTest]]: [MySQL] 55,554 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new850 bytes
new133.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1886448-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new134.88 KB
FAILED: [[SimpleTest]]: [MySQL] 53,999 pass(es), 40 fail(s), and 23 exception(s).
[ View ]

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

StatusFileSize
new428 bytes
new135.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1886448-86-mega-debugging.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

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.

Assigned:sun» Unassigned
Status:Needs work» Needs review
StatusFileSize
new135.56 KB
PASSED: [[SimpleTest]]: [MySQL] 55,914 pass(es).
[ View ]

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

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? :(

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).

StatusFileSize
new134.32 KB
FAILED: [[SimpleTest]]: [MySQL] 56,942 pass(es), 5 fail(s), and 2 exception(s).
[ View ]

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.

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.

#96 this... +1000 totally agreed

hook_theme_suggestions_alter rocks.

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.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

rewrite with some context :)

Issue summary:View changes

added back C4rl's render api link

Issue summary:View changes

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

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)

Status:Needs work» Needs review
StatusFileSize
new16.33 KB
new137.47 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new522 bytes
new137.51 KB
FAILED: [[SimpleTest]]: [MySQL] 56,574 pass(es), 115 fail(s), and 7,082 exception(s).
[ View ]

Ups. This might fix the installer.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.57 KB
new137.7 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new137.74 KB
FAILED: [[SimpleTest]]: [MySQL] 55,389 pass(es), 102 fail(s), and 218 exception(s).
[ View ]

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.

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.

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.

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

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.

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.

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

Issue summary:View changes

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

Issue summary:View changes
Issue tags:+phpunit
StatusFileSize
new63.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,382 pass(es), 4 fail(s), and 3 exception(s).
[ View ]

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.

Status:Needs work» Needs review

  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.

Status:Needs work» Needs review
StatusFileSize
new69.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,848 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new12.76 KB

This could be green now.

Status:Needs review» Needs work

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

StatusFileSize
new948 bytes
new69.82 KB
PASSED: [[SimpleTest]]: [MySQL] 59,969 pass(es).
[ View ]

Fixed the last failure.

  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.

StatusFileSize
new67.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,077 pass(es).
[ View ]
new11.37 KB

Fixed the points.

Status:Needs review» Reviewed & tested by the community

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

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

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

  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

Status:Reviewed & tested by the community» Needs work

#131 means that this needs work :)

Status:Needs work» Needs review
StatusFileSize
new8.48 KB
new71.13 KB
PASSED: [[SimpleTest]]: [MySQL] 58,046 pass(es).
[ View ]

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.

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.

Status:Needs work» Needs review

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

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.

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! :)

Title:Rewrite the theme registry into a proper serviceChange 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!

Status:Active» Needs review

Title:Change notice: Rewrite the theme registry into a proper serviceRewrite 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.

Tthank you jibran and Marc Carver for these updates.

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

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.

Issue tags:+theme service

retro adding the theme service tag