Not strictly needed to remove the registry but it would be nice to have this in :). Needs to happen anyway...

CommentFileSizeAuthor
#9 theme_registry_psr0_done_right_for_real.patch11.41 KBaspilicious
#6 theme_registry_psr0_done_right.patch10.9 KBamateescu
theme_registry_psr0.patch11.31 KBaspilicious
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Hmm this is not possible?

amateescu’s picture

amateescu CreditAttribution: amateescu commented 
+++ b/core/includes/theme.inc
@@ -1,5 +1,5 @@
-use Drupal\Core\Utility\CacheArray;
+

@@ -8,6 +8,8 @@ use Drupal\Core\Utility\CacheArray;
+use Core\Utility\ThemeRegistry;
+

See the problem? :)

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
@@ -0,0 +1,118 @@
+
+/**
+ * @file
+ * Definition of Core\Utility\ThemeRegistry
+ */
+
+namespace Core\Utility;

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/RegistryTest.php
@@ -8,7 +8,7 @@
 use Drupal\simpletest\WebTestBase;
-use ThemeRegistry;
+use Core\Utility\ThemeRegistry;

Same here.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented 
+++ b/core/includes/theme.inc
@@ -1,5 +1,5 @@
-use Drupal\Core\Utility\CacheArray;

This is removed correctly here, but this line isn't included in the new core/lib/Drupal/Core/Utility/ThemeRegistry.php
That will the cause of at least some failures.

+++ b/core/includes/theme.inc
@@ -8,6 +8,8 @@ use Drupal\Core\Utility\CacheArray;
+use Core\Utility\ThemeRegistry;

It seems ThemeRegistry is not used anywhere in the file?

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

I don't need to add "use Drupal\Core\Utility\CacheArray;" because I'm alrdy in the Drupal\Core\Utility namespace.
And "_theme_load_registry" uses the ThemeRegistry.

I just think we can't move this to psr-0 yet because of the stupid drupal bootstrap process that initializes the autoloader to late.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Component: other » theme system
Status: Needs work » Needs review
Issue tags: +PSR-0
FileSize
theme_registry_psr0_done_right.patch10.9 KB

@aspilicious, oh really?

@tstoeckler:
1) should be there now
2) it is used in theme.inc:337 (with the patch applied, don't know the line number without the patch)

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.phpundefined
@@ -0,0 +1,120 @@
+use Drupal\Core\Utility\CacheArray;

This isn't needed as I said before. We are alrdy in the Drupal\Core\Utility namespace...

15 days to next Drupal core point release.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

AAAAAH I forgot to add Drupal in the namespace. So this is needs work because of what I said earlier.
I'll reroll this.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
theme_registry_psr0_done_right_for_real.patch11.41 KB

Thanks for the fast reviews and the patch reroll else I wouldn't have seen my mistake.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community

Nicely done, all. The ThemeRegistry test is passing green, and the test bot is happy.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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