Introduce new Theme extension object and properly deprecate REGIONS_VISIBLE and REGIONS_ALL [#3015812] | Drupal.org
Skip to search
Can we use first and third party cookies and web beacons to
understand our audience, and to tailor promotions you see
Introduce new Theme extension object and properly deprecate REGIONS_VISIBLE and REGIONS_ALL
Fixed
Project:
Drupal core
Version:
11.x-dev
Component:
extension system
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Extension system
Theme System Modernization Initiative
@deprecated
LutskGCW22
ContributionWeekend2022
no-needs-review-bot
Reporter:
alexpott
Created:
23 Nov 2018 at 20:53 UTC
Updated:
21 Apr 2026 at 16:37 UTC
Jump to comment:
Most recent
Most recent file
Problem/Motivation
#2807785: Move global constants from *.module files into interfaces
deprecated a bunch of constants but it did not actually replace their usage. We should do this. This issue handles
REGIONS_VISIBLE
, and
REGIONS_ALL
. This is a bug because we've deprecated something but we've not completed the task and, more importantly, the old constants are in the System module and used by it but the new constants are in the Block module. That does not work.
Regions are properties of themes not blocks. They are declared in a theme's .info.yml file in the
regions
key and hidden via the
regions_hidden
key.
Proposed resolution
Current solution
Add a new
Theme
class that extends Extension and replaces the extension object in theme lists.
See API changes for a full scope of all the changes.
Other possible solutions
Add a new ThemeRegion service.
Add an object just to handle the theme extension's info array
- discounted in
#24
- we have code that depends on this being an array.
Add
ThemeHandler::listAllRegions($name)
ThemeHandler::listVisibleRegions($name)
and
ThemeHandler::getDefaultRegion($name)
- discounted in
#13
Remaining tasks
User interface changes
None
API changes
The main changes are:
system_region_list()
use
\Drupal::service('theme_handler')->getTheme()->listAllRegions()
or
\Drupal::service('theme_handler')->getTheme()->listVisibleRegions()
instead.
system_default_region()
use
\Drupal::service('theme_handler')->getTheme()->getDefaultRegion()
instead.
REGIONS_ALL
REGIONS_VISIBLE
\Drupal\block\BlockRepositoryInterface::REGIONS_ALL
and
\Drupal\block\BlockRepositoryInterface::REGIONS_VISIBLE
are deprecated and should not be used.
As a result several objects that had helper methods to the global functions also have API deprecations:
\Drupal\block\BlockListBuilder::systemRegionList()
use
$this->themeHandler->getTheme()->listAllRegions()
or
$this->themeHandler->getTheme()->listVisibleRegions()
instead.
\Drupal\block\Controller\BlockController::getVisibleRegionNames()
use
$this->themeHandler->getTheme()->listVisibleRegions()
instead.
\Drupal\block_place\Plugin\DisplayVariant\PlaceBlockPageVariant::getVisibleRegionNames()
use
$this->themeHandler->getTheme()->listVisibleRegions()
instead.
Several methods on the
ThemeHandler
return an array of
Theme
objects instead of
Extension
objects. As
Theme
extends from
Extension
this is allowed. These methods are:
\Drupal\Core\Extension\ThemeHandlerInterface::listInfo()
\Drupal\Core\Extension\ThemeHandlerInterface::rebuildThemeData()
\Drupal\Core\Extension\ThemeHandlerInterface::getTheme()
now returns a
Theme
object.
The one "breaking" change is
\Drupal\Core\Extension\ThemeHandlerInterface::addTheme()
which now only accepts a
Theme
object. This change is worth it because this is an incredibly low level method that only exists so that maintenance pages and install pages can use themes before anything is installed. The hard break would help any custom or contrib code in the very unlikely event that they are using it.
New
\Drupal\Core\Extension\Theme
that extends
\Drupal\Core\Extension\Extension
and we can build up the abilities of over time so eventually we can replace everything with methods. No more
$theme->info['foo']
and we can remove the public properties and turn them into value objects.
Data model changes
None
Comment
File
Size
Author
#173
3015812-nr-bot_e0ovh3_c.txt
91 bytes
needs-review-queue-bot
#171
3015812-nr-bot_damzgl84.txt
91 bytes
needs-review-queue-bot
#133
3015812-nr-bot.txt
147 bytes
needs-review-queue-bot
#130
3015812-130.patch
43.97 KB
ameymudras
#127
3015812-127.patch
44.08 KB
viappidu
#126
Screenshot 2022-11-23 at 1.42.21 AM.png
73.35 KB
akram khan
#125
reroll_diff_123-125.txt
22.54 KB
akram khan
#125
3015812-125.patch
32.66 KB
akram khan
#123
3015812-123.patch
44.54 KB
smustgrave
#123
interdiff-121-123.txt
2.42 KB
smustgrave
#121
interdiff-3015812-120-121.txt
1.59 KB
voleger
#121
3015812-121.patch
44.93 KB
voleger
#120
interdiff-3015812-118-120.txt
5.75 KB
voleger
#120
3015812-120.patch
44.93 KB
voleger
#118
3015812-118.patch
41 KB
smustgrave
#118
interdiff-116-118.txt
1.02 KB
smustgrave
#116
3015812-117.patch
42.35 KB
andypost
#116
interdiff.txt
539 bytes
andypost
#115
interdiff.txt
502 bytes
andypost
#114
3015812-114.patch
42.35 KB
andypost
#114
interdiff.txt
502 bytes
andypost
#111
3015812-111.patch
41.86 KB
smustgrave
#111
interdiff-108-111.txt
1.55 KB
smustgrave
#108
3015812-108.patch
41.91 KB
smustgrave
#108
interdiff-107-108.txt
2.04 KB
smustgrave
#107
interdiff-3015812-103-107.txt
10.92 KB
voleger
#107
3015812-107.patch
41.9 KB
voleger
#103
3015812-103.patch
41.81 KB
andypost
#103
interdiff.txt
11.75 KB
andypost
#100
interdiff-3015812-94-100.txt
1.96 KB
voleger
#100
3015812-100.patch
42.24 KB
voleger
#94
interdiff-3015812-93-94.txt
1.78 KB
voleger
#94
3015812-94.patch
42.25 KB
voleger
#93
interdiff-3015812-91-93.txt
2.96 KB
voleger
#93
3015812-93.patch
42.24 KB
voleger
#91
interdiff-3015812-89-91.txt
7.07 KB
voleger
#91
3015812-91.patch
42.17 KB
voleger
#89
interdiff-3015812-88-89.txt
1.33 KB
voleger
#89
3015812-89.patch
37.33 KB
voleger
#88
interdiff-3015812-87-88.txt
1.06 KB
voleger
#88
3015812-88.patch
37.34 KB
voleger
#87
3015812-87.patch
37.24 KB
voleger
#85
interdiff-3015812-82-85.txt
0 bytes
voleger
#85
3015812-85.patch
36.44 KB
voleger
#82
3015812-82.patch
35.7 KB
jofitz
#80
3015812-80.patch
35.56 KB
nikitagupta
#77
3015812-77.patch
35.65 KB
suresh prabhu parkala
#72
3015812-72.patch
37.45 KB
hardik_patel_12
#66
3015812-66.patch
41.07 KB
ravi.shankar
#63
interdiff-3015812-61-63.txt
10.53 KB
voleger
#63
3015812-63.patch
41.91 KB
voleger
#61
3015812-2-61.patch
42.22 KB
vacho
#48
3015812-2-48.patch
42.2 KB
alexpott
#48
46-48-interdiff.txt
4.75 KB
alexpott
#46
3015812-2-45.patch
43.76 KB
alexpott
#46
44-45-interdiff.txt
678 bytes
alexpott
#44
3015812-2-44.patch
43.72 KB
alexpott
#44
41-44-interdiff.txt
3.45 KB
alexpott
#41
3015812-2-41.patch
43.75 KB
alexpott
#40
3015812-2-40.patch
43.72 KB
alexpott
#39
3015812-2-39.patch
44.41 KB
alexpott
#39
36-39-interdiff.txt
1.6 KB
alexpott
#36
3015812-2-36.patch
43.23 KB
alexpott
#34
3015812-2-34.patch
40.69 KB
alexpott
#34
31-34-interdiff.txt
1.42 KB
alexpott
#31
3015812-2-31.patch
39.27 KB
alexpott
#31
27-31-interdiff.txt
1.8 KB
alexpott
#28
3015812-2-27.patch
37.51 KB
alexpott
#28
26-27-interdiff.txt
2.01 KB
alexpott
#27
3015812-2-27.patch
37.51 KB
alexpott
#24
3015812-24.patch
33.8 KB
alexpott
#24
23-24-interdiff.txt
758 bytes
alexpott
#23
3015812-23.patch
33.95 KB
alexpott
#23
21-23-interdiff.txt
2.09 KB
alexpott
#21
3015812-21.patch
33.24 KB
alexpott
#21
17-21-interdiff.txt
1.62 KB
alexpott
#17
3015812-17.patch
31.37 KB
alexpott
#10
3015812-10.patch
27.89 KB
alexpott
#10
8-10-interdiff.txt
3.19 KB
alexpott
#8
3015812-8.patch
24.7 KB
alexpott
#8
7-8-interdiff.txt
16.85 KB
alexpott
#8
2-8-interdiff.txt
5.94 KB
alexpott
#7
3015812-7.patch
23.39 KB
andypost
#7
interdiff.txt
631 bytes
andypost
#6
3015812-6.patch
23.36 KB
andypost
#6
interdiff.txt
1.3 KB
andypost
#5
3015812-5.patch
23.24 KB
andypost
#4
interdiff.txt
13.15 KB
andypost
#5
interdiff.txt
3.32 KB
andypost
#4
3015812-4.patch
23.24 KB
andypost
#2
3015812-2.patch
22.53 KB
alexpott
Issue fork
drupal-3015812
Show commands
Start within a Git clone of the project using the
version control instructions
Add & fetch this issue fork’s repository
Or,
if you do not have
SSH keys set up on git.drupalcode.org
Add & fetch this issue fork’s repository
2 hidden branches
3015812-introduce-new-theme-11.x
plain diff
MR
!15506
Check out this branch for the first time
Check out existing branch, if you already have it locally
3015812-introduce-new-theme
plain diff
MR
!10369
Check out this branch for the first time
Check out existing branch, if you already have it locally
About issue forks
Comments
Comment
#1
23 November 2018 at 20:53
alexpott
created an issue. See
original summary
or
to post comments
Comment
#2
alexpott
he/they
🇪🇺🌍
commented
24 November 2018 at 00:15
Status:
Active
» Needs review
Issue tags:
Needs issue summary update
Status
File
Size
new
3015812-2.patch
22.53 KB
I think we should re-consider how we have deprecated REGIONS_ALL and REGIONS_VISIBLE. Regions are properties of themes not blocks. Yes blocks are placed in regions but the list of regions are determined by the theme not blocks.
The patch attached deprecated system_region_list() and system_default_region() and places the methods on the ThemeHandler - that because they are easiest to implement there and as you can see from some of the changes there are places where with what is already injected into objects this makes sense. I'm not sure - other ideas more than welcome.
or
to post comments
Comment
#3
andypost
he/him
Russian
commented
24 November 2018 at 17:00
Status:
Needs review
» Needs work
Issue tags:
@deprecated
That looks great and much cleaner
Maybe instead of
new Translatable($label)
extension api could provide that
+++ b/core/modules/block/src/BlockListBuilder.php
@@ -404,6 +418,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
protected function systemRegionList($theme, $show = REGIONS_ALL) {
+ @trigger_error('@todo', E_USER_DEPRECATED);
+++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
@@ -133,7 +143,8 @@ public function build() {
protected function getVisibleRegionNames($theme) {
...
+ @trigger_error('@todo', E_USER_DEPRECATED);
+++ b/core/modules/system/system.module
@@ -1062,25 +1064,18 @@ function system_rebuild_module_data() {
function system_region_list($theme, $show = REGIONS_ALL) {
...
+ @trigger_error('@todo', E_USER_DEPRECATED);
@@ -1127,10 +1122,12 @@ function system_system_info_alter(&$info, Extension $file, $type) {
function system_default_region($theme) {
...
+ @trigger_error('@todo', E_USER_DEPRECATED);
also needs testing for deprecated BC + exception
or
to post comments
Comment
#4
andypost
he/him
Russian
commented
24 November 2018 at 17:39
Status
File
Size
new
interdiff.txt
13.15 KB
new
3015812-4.patch
23.24 KB
Added
regions_hidden
to theme init
As all of this methods getters - then let's call them not "list"
A bit of clean-up
or
to post comments
Comment
#5
andypost
he/him
Russian
commented
24 November 2018 at 22:42
Status:
Needs work
» Needs review
Status
File
Size
new
interdiff.txt
3.32 KB
new
3015812-5.patch
23.24 KB
Clean-up renaming
or
to post comments
Comment
#6
andypost
he/him
Russian
commented
24 November 2018 at 23:36
Status
File
Size
new
interdiff.txt
1.3 KB
new
3015812-6.patch
23.36 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-4.patch
23.24 KB
hidden
3015812-5.patch
23.24 KB
Deletegated in theme init, hidden is not used in active theme so every usage should use theme handler
or
to post comments
Comment
#7
andypost
he/him
Russian
commented
25 November 2018 at 00:07
Status
File
Size
new
interdiff.txt
631 bytes
new
3015812-7.patch
23.39 KB
1 file was hidden/shown/deleted
Status
File
Size
hidden
3015812-6.patch
23.36 KB
make translation inline with core
or
to post comments
Comment
#8
alexpott
he/they
🇪🇺🌍
commented
25 November 2018 at 06:12
Status
File
Size
new
2-8-interdiff.txt
5.94 KB
new
7-8-interdiff.txt
16.85 KB
new
3015812-8.patch
24.7 KB
@andypost I think listBlah() is great for a getter that returns an array. But whatevs we don't have a standard here.
I think
#6
and any changes to ActiveTheme is out-of-scope and should be done in another issue.
#7
is also out-of-scope.
+++ b/core/modules/system/system.module
@@ -1062,25 +1064,18 @@ function system_rebuild_module_data() {
- $list[$name] = t($label);
This is the original code... the Layout stuff has added
'context' => 'layout_region'
but that is an experimental module and there needs to be separate issue to introduce this to system_region_list().
All told I'm going to revert to
#2
with improvements to block_rebuild() from
#4
Added a new change record and started to write it
and added proper deprecation notices.
I still think we need to discuss whether the ThemeHandler is the correct place for this. For me a potentially better way would a Theme object that was built from the Extension object so you could do something like
$this->themeHandler->getTheme($name)->listAllRegions()
but this requires massive changes to the the theme listing system which are afoot in
#2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList
but even those changes have not yet given us properly typed Extensions objects ie Module / Theme / Profile. So maybe this is the best we can do at the moment. I think that changes to
block_rebuild()
show that putting this on the theme handler is not that bad a compromise.
Need to add deprecation tests.
or
to post comments
Comment
#9
alexpott
he/they
🇪🇺🌍
commented
25 November 2018 at 06:24
I guess the other option is to have a ThemeRegion service. It would depend on the theme_handler and string_translation services.
or
to post comments
Comment
#10
alexpott
he/they
🇪🇺🌍
commented
25 November 2018 at 06:45
Status
File
Size
new
8-10-interdiff.txt
3.19 KB
new
3015812-10.patch
27.89 KB
Here's a test for region listing both via theme_handler and the legacy methods.
or
to post comments
Comment
#11
alexpott
he/they
🇪🇺🌍
commented
25 November 2018 at 06:52
Issue summary:
View changes
Issue tags:
Needs issue summary update
Updated issue summary.
or
to post comments
Comment
#12
alexpott
he/they
🇪🇺🌍
commented
25 November 2018 at 06:53
Issue summary:
View changes
or
to post comments
Comment
#13
tim.plunkett
he/him
Philadelphia
commented
25 November 2018 at 17:33
the other option is to have a ThemeRegion service. It would depend on the theme_handler and string_translation services.
This sounds preferable, and also avoiding a BC break by not expanding ThemeHandlerInterface.
or
to post comments
Comment
#14
andypost
he/him
Russian
commented
25 November 2018 at 18:40
Tim do you have any thoughts about how to swap it later for
#2924058: Discuss using Layout Builder to control full site layout and replace Block UI
or
to post comments
Comment
#15
andypost
he/him
Russian
commented
25 November 2018 at 18:47
@alexpott what if we add extension getFeatures() which will return specific extension info.yml parsing and validation object - surely followup
On my side I'd like to split theme info which is already dumplicated in activetheme class object
or
to post comments
Comment
#16
tim.plunkett
he/him
Philadelphia
commented
25 November 2018 at 18:48
Making it a new service could probably help with that (as Layout Builder could swap out the service), but that issue isn't a current focus of the Layout Initiative
or
to post comments
Comment
#17
alexpott
he/they
🇪🇺🌍
commented
26 November 2018 at 12:22
Issue summary:
View changes
Status
File
Size
new
3015812-17.patch
31.37 KB
Not sure about this being a service. I prefer the
Somehow move this a Theme extension object (not sure this is achievable).
option.
Here's a patch doing that. It introduces an ThemeInfo value object that handles the
->info
part of the Theme extension object. That's let's us have methods :)
No interdiff because it is a fresh approach.
or
to post comments
Comment
#18
alexpott
he/they
🇪🇺🌍
commented
26 November 2018 at 12:24
Issue summary:
View changes
or
to post comments
Comment
#19
alexpott
he/they
🇪🇺🌍
commented
26 November 2018 at 12:27
Issue summary:
View changes
or
to post comments
Comment
#20
26 November 2018 at 13:13
Status:
Needs review
» Needs work
The last submitted patch,
17: 3015812-17.patch
, failed testing.
View results
or
to post comments
Comment
#21
alexpott
he/they
🇪🇺🌍
commented
26 November 2018 at 13:42
Status:
Needs work
» Needs review
Status
File
Size
new
17-21-interdiff.txt
1.62 KB
new
3015812-21.patch
33.24 KB
Accounting for the case where a block exists for a theme that is not installed :(
or
to post comments
Comment
#22
26 November 2018 at 14:29
Status:
Needs review
» Needs work
The last submitted patch,
21: 3015812-21.patch
, failed testing.
View results
or
to post comments
Comment
#23
alexpott
he/they
🇪🇺🌍
commented
26 November 2018 at 14:41
Status:
Needs work
» Needs review
Status
File
Size
new
21-23-interdiff.txt
2.09 KB
new
3015812-23.patch
33.95 KB
We need PHP 7.1's iterable type hint :( oh well. This is work aroundable.
or
to post comments
Comment
#24
alexpott
he/they
🇪🇺🌍
commented
26 November 2018 at 15:29
Status
File
Size
new
23-24-interdiff.txt
758 bytes
new
3015812-24.patch
33.8 KB
Doh.
or
to post comments
Comment
#25
26 November 2018 at 15:33
The last submitted patch,
23: 3015812-23.patch
, failed testing.
View results
or
to post comments
Comment
#26
26 November 2018 at 16:18
Status:
Needs review
» Needs work
The last submitted patch,
24: 3015812-24.patch
, failed testing.
View results
or
to post comments
Comment
#27
alexpott
he/they
🇪🇺🌍
commented
27 November 2018 at 11:38
Status:
Needs work
» Needs review
Status
File
Size
new
3015812-2-27.patch
37.51 KB
Okay so array_intersect_key() doesn't work on \ArrayAccess objects :(
Here's another approach that confers quite a few advantages. It adds a Theme extension object that extends from Extension and documents the public properties. This will allow us to properly deprecate them and move to a value object.
No interdiff because it'd be every single line.
or
to post comments
Comment
#28
alexpott
he/they
🇪🇺🌍
commented
27 November 2018 at 12:16
Status
File
Size
new
26-27-interdiff.txt
2.01 KB
new
3015812-2-27.patch
37.51 KB
Fixing some tests from
#27
or
to post comments
Comment
#29
27 November 2018 at 12:26
The last submitted patch,
27: 3015812-2-27.patch
, failed testing.
View results
or
to post comments
Comment
#30
alexpott
he/they
🇪🇺🌍
commented
27 November 2018 at 12:30
Issue summary:
View changes
Updated the issue summary with the current state.
or
to post comments
Comment
#31
alexpott
he/they
🇪🇺🌍
commented
27 November 2018 at 12:35
Status
File
Size
new
27-31-interdiff.txt
1.8 KB
new
3015812-2-31.patch
39.27 KB
Ignore the patch in
#28
it's the same as
#27
:( - here are the fixes.
or
to post comments
Comment
#32
27 November 2018 at 12:36
The last submitted patch,
28: 3015812-2-27.patch
, failed testing.
View results
or
to post comments
Comment
#33
27 November 2018 at 13:24
Status:
Needs review
» Needs work
The last submitted patch,
31: 3015812-2-31.patch
, failed testing.
View results
or
to post comments
Comment
#34
alexpott
he/they
🇪🇺🌍
commented
27 November 2018 at 14:02
Status:
Needs work
» Needs review
Status
File
Size
new
31-34-interdiff.txt
1.42 KB
new
3015812-2-34.patch
40.69 KB
So the
LanguageBlockSettingsVisibilityTest
is using a theme that is not installed. It shouldn't be doing that the link is not available in the UI and it would be impossible to add a block for a theme that is not installed because you'll get a dependency error.
or
to post comments
Comment
#35
alexpott
he/they
🇪🇺🌍
commented
29 November 2018 at 00:10
#2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList
is back to RTBC - we should definitely do that one first as it is more important and will mean lots of changes to this one.
or
to post comments
Comment
#36
alexpott
he/they
🇪🇺🌍
commented
29 November 2018 at 15:25
Issue tags:
Needs issue summary update
Status
File
Size
new
3015812-2-36.patch
43.23 KB
Here's a rebase on top of
#2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList
Need to update the issue summary a little with the impact. No interdiff because the changes are extensive and need to be reviewed in the context on the new theme extension list.
or
to post comments
Comment
#37
alexpott
he/they
🇪🇺🌍
commented
29 November 2018 at 15:26
Also this incorporates
#3016968: Remove weight public property from theme extensions
or
to post comments
Comment
#38
29 November 2018 at 16:15
Status:
Needs review
» Needs work
The last submitted patch,
36: 3015812-2-36.patch
, failed testing.
View results
or
to post comments
Comment
#39
alexpott
he/they
🇪🇺🌍
commented
29 November 2018 at 16:37
Status:
Needs work
» Needs review
Status
File
Size
new
36-39-interdiff.txt
1.6 KB
new
3015812-2-39.patch
44.41 KB
or
to post comments
Comment
#40
alexpott
he/they
🇪🇺🌍
commented
30 November 2018 at 12:04
Status
File
Size
new
3015812-2-40.patch
43.72 KB
Rerolled now the weight patch landed.
or
to post comments
Comment
#41
alexpott
he/they
🇪🇺🌍
commented
14 January 2019 at 13:55
Title:
Properly deprecate REGIONS_VISIBLE and REGIONS_ALL
» Introduce new Theme extension object and properly deprecate REGIONS_VISIBLE and REGIONS_ALL
Issue summary:
View changes
Issue tags:
Needs issue summary update
Status
File
Size
new
3015812-2-41.patch
43.75 KB
Let's kick another test off - I rebased my branch on the latest 8.7.x... no changes to
#40
The issue summary is up-to-date with the current patch. Changed title to reflect scope.
or
to post comments
Comment
#42
phenaproxima
he/him
Massachusetts
commented
14 January 2019 at 18:10
+++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
@@ -311,7 +311,8 @@ protected function doList() {
+ $extension = $this->decorateExtension($extension);
+ $extensions[$extension_name] = $extension;
Nit: Why split this into two lines?
+++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
@@ -540,25 +541,27 @@ public function getPath($extension_name) {
+ * extensions info.yml information. This maybe be overridden to return an
Should be "This may be overridden..."
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ * Whether the Theme is installed or not.
Nit: "Theme" should be "theme".
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ * @todo Remove all code that relies on the $status property.
+ *
+ * @var int
+ */
+ public $status;
To facilitate the removal of the property, maybe this would be a good time to add an isEnabled() method to replace it?
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ /**
+ * The info array based on the theme's .info.yml file.
+ *
+ * @var array
+ */
+ public $info;
Should this default to an empty array?
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ /**
+ * The relative path to the theme engine extension file.
+ *
+ * @var string
+ */
+ public $owner;
"Owner" seems like a strange name for this property. Maybe we should take this opportunity to rename it and add a magic __get() wrapper around "owner", if external code relies on it.
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ /**
+ * The internal name of the theme engine extension
+ *
+ * @var string
+ */
+ public $prefix;
Same here.
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ * @param int $status
+ * The theme's installation status.
Should this be a boolean?
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ public function __construct($root, Extension $extension, array $info, $status) {
Passing an Extension object to this constructor seems strange. It almost feels like it should be a static createFromExtension() method...
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ if ($this->info['base theme'] === FALSE) {
+ unset($this->info['base theme']);
+ }
+ else {
+ // Add the base theme as a proper dependency.
+ $this->info['dependencies'][] = $this->info['base theme'];
+ }
What if 'base theme' is an empty string, or null? Maybe this should be an empty() check instead.
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ public function getDefaultRegion() {
+ return (string) key($this->listVisibleRegions());
+ }
Why does the return value need to be casted to a string?
+++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
@@ -104,19 +67,39 @@ public function __construct($root, $type, CacheBackendInterface $cache, InfoPars
+ $engines = $this->engineList->getList();
Where is this used?
+++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
@@ -245,24 +226,10 @@ protected function doGetBaseThemes(array $themes, $theme, array $used_themes = [
+ $status = (int) isset($this->installedThemes[$extension->getName()]);
Isn't there a method to retrieve the list of installed themes?
or
to post comments
Comment
#43
markhalliwell
commented
14 January 2019 at 18:38
Status:
Needs review
» Needs work
Issue tags:
Extension system
, +
Theme System Modernization Initiative
A little overlap from
#42
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ /**
+ * Whether the Theme is installed or not.
+ *
+ * @todo Remove all code that relies on the $status property.
+ *
+ * @var int
+ */
+ public $status;
+ /**
+ * The info array based on the theme's .info.yml file.
+ *
+ * @var array
+ */
+ public $info;
These should really be a part of
Extension
, they're not theme specific:
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,167 @@
+ public function __construct($root, Extension $extension, array $info, $status) {
+ if ($extension->getType() !== 'theme') {
+ throw new \InvalidArgumentException('Constructing a Theme object using an extension that is not a theme makes no sense');
+ }
+ parent::__construct($root, 'theme', $extension->getPathname(), $extension->getExtensionFilename());
+ $this->info = $info + static::getInfoDefaults();
This is really weird and feels like a huge anti-pattern.
The
Theme
object extends from
Extension
, it shouldn't need an
Extension
object to create a
Theme
object.
+++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
@@ -245,24 +226,10 @@ protected function doGetBaseThemes(array $themes, $theme, array $used_themes = [
+ protected function decorateExtension(Extension $extension) {
+ $info = $this->infoParser->parse($extension->getPathname());
+ $status = (int) isset($this->installedThemes[$extension->getName()]);
+ return new Theme($this->root, $extension, $info, $status);
Decorating something shouldn't return a whole new object.
I'm also wondering if maybe this shouldn't be postponed on
#2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation
and
#3023131: [PP-1] Extension System, Part IV: ExtensionHandler and ExtensionHandlerInterface
. These issues will clear up a lot of how extensions are constructed/handled by
ExtensionList
and eliminate some of the weird hackery that's going on currently and the extended proposed changes that only add to the nightmare.
or
to post comments
Comment
#44
alexpott
he/they
🇪🇺🌍
commented
16 January 2019 at 10:59
Status:
Needs work
» Needs review
Status
File
Size
new
41-44-interdiff.txt
3.45 KB
new
3015812-2-44.patch
43.72 KB
Thanks for the reviews
Re
#42
Because we don't want to pass the array version into
$this->moduleHandler->alter('system_info', $extension->info, $extension, $this->type);
- you are only supposed to be able to alter the info - not the extension object itself.
Fixed
Fixed
I suggest we do that in followups because then we can use the deprecated property trait and concentrate on each property - this issue is doing enough
Can't hurt
Follow-up material for me and we should replace with a method so /shrug about renaming the property
Follow-up material for me and we should replace with a method so /shrug about renaming the property
This is BC. /shrug it is not biggie to me - extensions used to have a tri-state status enabled/disabled/uninstalled so perhaps this is hang up from then. Needs more discussion if we change it here.
Changed this to no longer need an extension - less complexity now.
No this is a very careful copy of the existing logic and it needs to be maintained.
To conform to the interface in the super super rare case that the theme has no visible regions.
key([])
returns NULL. Again this is BC it is what system_default_region() does with
return isset($regions[0]) ? $regions[0] : '';
Lower down in that method we do
if (isset($engines[$engine])) {
$theme->owner = $engines[$engine]->getExtensionPathname();
$theme->prefix = $engines[$engine]->getName();
Yes. But still we need to maintain this property for BC - we could decide to get rid of it and tell people to use the get all installed method BUT I think the extension listing service should return extension objects which have the ability to know whether they are installed - this leads to very natural code.
RE
#43
There is nothing to stop us moving them up a class in a follow-up and I agree but issue scope.
I think you like the words "anti pattern" too much. But anyway the principle of taking an object and decorating it is not one. As it is, I've refactored this out because we can have less complexity by not doing it.
Yes it should. For example, this is exactly how decorating a service in Symfony works. Or our middleware. Or well any other implementation of the decorator pattern. What is different about this one is that we’re not storing the decorated extension object because that is not necessary. Anyhow, moot see 2.
For me this shouldn't be postponed on
#2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation
that is make extensive changes whereas this is a small change in the object returned from the extension list which allows us to gives these extension object type specific functionality which is a nice change because it makes code easier to understand and will allow us to finally move forward and refactor the public properties we've been jamming on to this object for too long.
or
to post comments
Comment
#45
markhalliwell
commented
16 January 2019 at 13:01
#43.1
There is nothing to stop us moving them up a class in a follow-up and I agree but issue scope.
Not entirely sure how this is issue scope when it's introducing official properties that belong to the parent class in the first place.
#43.2
I think you like the words "anti pattern" too much. But anyway the principle of taking an object and decorating it is not one.
I don't use it that much lol I wasn't aware that you were attempting to turn the Extension objects into decorated classes, which is why it looked quite odd for a sub-class to require a parent class. I tend to think of Extensions as stand-alone objects; able to be created using scalar values.
#43.3
Yes it should.
Please forgive my ignorance, I'm still not all that familiar with decorators as they aren't as prevalent on the FE (yet). Regardless, if this were to be a decorator, I think it'd need that name appended to the class to clarify what it's supposed to be doing. Given that it wasn't, I think that's what confused me the most.
For me this shouldn't be postponed on
#2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation
that is make extensive changes...
True, I was thinking to postpone more on
#3023131: [PP-1] Extension System, Part IV: ExtensionHandler and ExtensionHandlerInterface
as that will likely change a lot of how Extensions are listed/handled as a whole.
edit: separating this out because I think it got overlooked.
Perhaps instead of postponing we should mark
Theme
as
@internal
temporarily?
I suspect this object will undergo quite a lot of changes over several issues once it's introduced.
My only real concern is introducing something that isn't yet fully flushed out for the sake of expediency.
or
to post comments
Comment
#46
alexpott
he/they
🇪🇺🌍
commented
16 January 2019 at 13:01
Status
File
Size
new
44-45-interdiff.txt
678 bytes
new
3015812-2-45.patch
43.76 KB
Added @todo to
#3026232: Deprecate Theme extension object public properties
to deprecate public property usage.
I've thought about making
\Drupal\Core\Extension\ExtensionDiscovery
return Theme objects instead of Extension objects but I think that that should continue to work with raw Extension object and leave decorating to the ExtensionList classes because:
In order to decorate properly you need to parse the info file
There's likely to be specific logic for theme or modules that's hard to get right during \Drupal\Core\Extension\ExtensionDiscovery::scanDirectory and we certainly don't want extension status bleeding down to that level.
or
to post comments
Comment
#47
dawehner
German
commented
16 January 2019 at 13:08
To be honest decoration is a really weird word. We do composition here and the extension has a clear limited scope. Inheritance isn't the only way how you can architect your data structures :)
or
to post comments
Comment
#48
alexpott
he/they
🇪🇺🌍
commented
16 January 2019 at 13:32
Status
File
Size
new
46-48-interdiff.txt
4.75 KB
new
3015812-2-48.patch
42.2 KB
@markcarver it is fine for us to move the public properties up to extension but there really is out of scope here because that will involve tackling the module system - we don't have to here so let's not.
Wrt to change on the Theme object at the moment in HEAD everything is determined from the info array, the extension object and the theme status - the patch does not change that. And I've not seen future plans to change that. So the important thing here is that we agree the names of the public methods added to the Theme object and what public methods to add because that's what would be hard to change. Adding new method in future releases as we deprecate the public properties and provide decent getters for other things is allowed.
The new methods are:
listAllRegions()
listVisibleRegions()
getDefaultRegion()
static getInfoDefaults() (ended up removing this)
static getDefaultFeatures() (ended up removing this)
I think the first 3 are well placed and named but I'm conflicted about the other two. Actually as
static getInfoDefaults()
is never used outside the class we could make it a class property and be done. And we could leave _system_default_theme_features() in place and handle in a follow-up if we like. I've done that in the patch attached. Nice less new API and we can add similar stuff in the future if we like.
@dawehner yeah it is tricky - in the \Drupal\Core\Extension\ExtensionList::decorateExtension() I think the future will be always replacing the extension object with something specific like Theme / Module / Profile / ThemeEngine but we're not there yet. But I agree we're not doing the decorator pattern here. And no we're not taking an Extension object in the constructor we're doing less of a decorator pattern. Maybe I should go back to doing a proper decorate pattern and hand off calls to the wrapped Extension object?
or
to post comments
Comment
#49
alexpott
he/they
🇪🇺🌍
commented
16 January 2019 at 13:40
Issue summary:
View changes
or
to post comments
Comment
#50
alexpott
he/they
🇪🇺🌍
commented
16 January 2019 at 13:44
So I think the important discussion to have is the name and documentation of
\Drupal\Core\Extension\ExtensionList::decorateExtension
Here's the default implementation:
/**
* Decorates an extension object.
* The default behavior is to add a public $info property containing the
* extensions info.yml information. This may be overridden to return an object
* that extends the Extension object.
* @param \Drupal\Core\Extension\Extension $extension
* The extension to be decorated.
* @return \Drupal\Core\Extension\Extension
* The decorated extension.
*/
protected function decorateExtension(Extension $extension) {
$info = $this->infoParser->parse($extension->getPathname());
// Add the info file modification time, so it becomes available for
// contributed extensions to use for ordering extension lists.
$info['mtime'] = $extension->getMTime();
// Merge extension type-specific defaults.
$extension->info = $info + $this->defaults;
return $extension;
Here's the theme override:
/**
* {@inheritdoc}
*/
protected function decorateExtension(Extension $extension) {
$info = $this->infoParser->parse($extension->getPathname());
$status = (int) isset($this->installedThemes[$extension->getName()]);
return new Theme($this->root, $extension->getPathname(), $extension->getExtensionFilename(), $info, $status);
In the future when all extension types have their own object I think we should consider deprecating the base implementation and marking it abstract in a future major release.
or
to post comments
Comment
#51
markhalliwell
commented
16 January 2019 at 14:50
I'm not sure what the name should be. Personally, I don't believe replacing
createExtensionInfo
is necessarily the right move here.
In fact,
$info['mtime'] = $extension->getMTime();
is missing from the
ThemeExtensionList
implementation.
Maybe it should be its own new method that invokes
createExtensionInfo
I know this is out of scope, but food for thought... ExtensionInfo from
#2186491: [meta] D8 Extension System: Discovery/Listing/Info
was pretty much sidelined and merged with ExtensionList (which is already doing way too much IMO). It may be beneficial to rethink introducing this in another issue?
---
I think we should temporarily mark Theme as @internal.
I suspect this object will undergo quite a lot of changes over several issues once it's introduced.
My only real concern is introducing something that isn't yet fully flushed out for the sake of expediency.
or
to post comments
Comment
#52
phenaproxima
he/him
Massachusetts
commented
16 January 2019 at 14:55
I think we should temporarily mark Theme as @internal.
+1 for this. I can't imagine too many people will extend or muck around with it, but we should at least make it as "sealed" and private as we realistically can, at least for now.
or
to post comments
Comment
#53
andypost
he/him
Russian
commented
16 January 2019 at 15:35
About naming field api using notion of "massage" to extend fields to widgets
or
to post comments
Comment
#54
alexpott
he/they
🇪🇺🌍
commented
16 January 2019 at 23:53
In fact, $info['mtime'] = $extension->getMTime(); is missing from the ThemeExtensionList implementation.
Not it's not - see Theme::__construct()
Wrt to marking it @internal - the problem with that is that is not really what I think you mean. People will interact with these objects. So they are not @internal - but what we don't what is for people to think this is an extensible API. So I think that we could argue that Theme should be final but the final debate is for another day because the topic of final seems contentious in the other issues it has come up in.
or
to post comments
Comment
#55
markhalliwell
commented
17 January 2019 at 00:35
Not it's not - see Theme::__construct()
That isn't ThemeExtensionList. I was referring to the code snippets in
#50
and how it's a little confusing that it splits the responsibility of populating the info array between ThemeExtensionList and Theme.
Wrt to marking it @internal - the problem with that is that is not really what I think you mean. People will interact with these objects.
People already interact with ExtensionList and it's marked @internal because we haven't yet finalized the service and for good reason. I don't see why we can't do that here as well. I don't see a reason why backing us into a corner is necessary just yet.
+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -112,7 +112,7 @@ public function listInfo() {
- public function addTheme(Extension $theme) {
+ public function addTheme(Theme $theme) {
+++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
@@ -95,10 +95,10 @@ public function listInfo();
- public function addTheme(Extension $theme);
+ public function addTheme(Theme $theme);
Ins't this technically a BC break (changing existing interfaces)?
or
to post comments
Comment
#56
andypost
he/him
Russian
commented
23 February 2019 at 20:11
Related issues:
#3035288: Deprecate theme_get_setting()
34 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-2.patch
22.53 KB
hidden
interdiff.txt
13.15 KB
hidden
interdiff.txt
3.32 KB
hidden
interdiff.txt
1.3 KB
hidden
interdiff.txt
631 bytes
hidden
3015812-7.patch
23.39 KB
hidden
2-8-interdiff.txt
5.94 KB
hidden
7-8-interdiff.txt
16.85 KB
hidden
3015812-8.patch
24.7 KB
hidden
8-10-interdiff.txt
3.19 KB
hidden
3015812-10.patch
27.89 KB
hidden
3015812-17.patch
31.37 KB
hidden
17-21-interdiff.txt
1.62 KB
hidden
3015812-21.patch
33.24 KB
hidden
21-23-interdiff.txt
2.09 KB
hidden
3015812-23.patch
33.95 KB
hidden
23-24-interdiff.txt
758 bytes
hidden
3015812-24.patch
33.8 KB
hidden
3015812-2-27.patch
37.51 KB
hidden
26-27-interdiff.txt
2.01 KB
hidden
3015812-2-27.patch
37.51 KB
hidden
27-31-interdiff.txt
1.8 KB
hidden
3015812-2-31.patch
39.27 KB
hidden
31-34-interdiff.txt
1.42 KB
hidden
3015812-2-34.patch
40.69 KB
hidden
3015812-2-36.patch
43.23 KB
hidden
36-39-interdiff.txt
1.6 KB
hidden
3015812-2-39.patch
44.41 KB
hidden
3015812-2-40.patch
43.72 KB
hidden
3015812-2-41.patch
43.75 KB
hidden
41-44-interdiff.txt
3.45 KB
hidden
3015812-2-44.patch
43.72 KB
hidden
44-45-interdiff.txt
678 bytes
hidden
3015812-2-45.patch
43.76 KB
or
to post comments
Comment
#57
claudiu.cristea
Romanian
Arad 🇷🇴
commented
25 February 2019 at 17:34
@alexpott, @andypost
Do you think that the actual
theme_get_setting()
could be moved as a method in the new
Theme
object, as it has been suggested in
#3035288-7: Deprecate theme_get_setting()
or
to post comments
Comment
#58
andypost
he/him
Russian
commented
25 February 2019 at 19:53
@claudiu.cristea I think it is because of encapsulation of this logic related exactly to theme - and Mark already explained it in
#3035288-11: Deprecate theme_get_setting()
So makes sense to figure
#2024043: Add Module, Theme, Profile, and Extension value objects
or
to post comments
Comment
#59
25 February 2019 at 19:53
Version:
8.7.x-dev
» 8.8.x-dev
Drupal 8.7.0-alpha1
will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the
Drupal 8 minor version schedule
and the
Allowed changes during the Drupal 8 release cycle
or
to post comments
Comment
#60
andypost
he/him
Russian
commented
27 July 2019 at 04:38
Status:
Needs review
» Needs work
Issue tags:
Needs reroll
or
to post comments
Comment
#61
vacho
commented
29 July 2019 at 20:52
Status
File
Size
new
3015812-2-61.patch
42.22 KB
I contrib to this cause rerolling.
or
to post comments
Comment
#62
vacho
commented
29 July 2019 at 20:53
Issue tags:
Needs reroll
or
to post comments
Comment
#63
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
2 September 2019 at 22:48
Status
File
Size
new
3015812-63.patch
41.91 KB
new
interdiff-3015812-61-63.txt
10.53 KB
Fix CS issues
or
to post comments
Comment
#64
ravi.shankar
commented
3 September 2019 at 03:48
Status:
Needs work
» Needs review
or
to post comments
Comment
#65
3 September 2019 at 03:48
Version:
8.8.x-dev
» 8.9.x-dev
Drupal 8.8.0-alpha1
will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the
Drupal 8 and 9 minor version schedule
and the
Allowed changes during the Drupal 8 and 9 release cycles
or
to post comments
Comment
#66
ravi.shankar
commented
30 November 2019 at 09:43
Status
File
Size
new
3015812-66.patch
41.07 KB
Here is a re-roll of patch #63.
or
to post comments
Comment
#67
gábor hojtsy
he/him
Hungarian
Hungary
commented
7 February 2020 at 15:59
Issue tags:
Needs release manager review
Hm, so in
#3111942: Remove all remaining @deprecated code from system module
I attempted to remove the region constants. They were deprecated with a suggestion to use
\Drupal\block\BlockRepositoryInterface::
instead. So if we are adding this new API in 8.9 that changes what the resolution for the deprecation is. How is that compatible with defining deprecations up until Drupal 8.8? Tagging for release manager review.
or
to post comments
Comment
#68
berdir
German
Switzerland
commented
8 February 2020 at 08:51
Yes, this missed the deadline and therefore the solution is likely to undeprecate that constant or change it to a D10 removal.
or
to post comments
Comment
#69
alexpott
he/they
🇪🇺🌍
commented
9 February 2020 at 12:54
Related issues:
#3112263: Undeprecate REGIONS_* constants
Opened
#3112263: Undeprecate REGIONS_* constants
to remove the deprecation. We can then do something like this in Drupal 9.
or
to post comments
Comment
#70
gábor hojtsy
he/him
Hungarian
Hungary
commented
10 February 2020 at 10:50
Version:
8.9.x-dev
» 9.1.x-dev
Moving to Drupal 9.1 then :)
or
to post comments
Comment
#71
daffie
commented
6 May 2020 at 07:48
Status:
Needs review
» Needs work
Issue tags:
Needs reroll
or
to post comments
Comment
#72
hardik_patel_12
commented
7 May 2020 at 16:45
Status:
Needs work
» Needs review
Status
File
Size
new
3015812-72.patch
37.45 KB
4 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-2-61.patch
42.22 KB
hidden
3015812-63.patch
41.91 KB
hidden
interdiff-3015812-61-63.txt
10.53 KB
hidden
3015812-66.patch
41.07 KB
Re-rolled for 9.1.x
or
to post comments
Comment
#73
ravi.shankar
commented
8 May 2020 at 04:13
Issue tags:
Needs reroll
Removed needs re-roll tag.
or
to post comments
Comment
#74
daffie
commented
8 May 2020 at 08:05
Status:
Needs review
» Needs work
The patch fails the testbot.
or
to post comments
Comment
#75
8 May 2020 at 08:05
Version:
9.1.x-dev
» 9.2.x-dev
Drupal 9.1.0-alpha1
will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the
Drupal 9 minor version schedule
and the
Allowed changes during the Drupal 9 release cycle
or
to post comments
Comment
#76
andypost
he/him
Russian
commented
22 April 2021 at 22:29
Issue tags:
Needs reroll
for 9.2.0
or
to post comments
Comment
#77
suresh prabhu parkala
commented
23 April 2021 at 09:25
Status:
Needs work
» Needs review
Status
File
Size
new
3015812-77.patch
35.65 KB
Re-roll against 9.2.x. Please review.
or
to post comments
Comment
#78
andypost
he/him
Russian
commented
23 April 2021 at 10:35
Status:
Needs review
» Needs work
+ * @expectedDeprecation system_region_list() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use
Wrong reroll
or
to post comments
Comment
#79
23 April 2021 at 10:35
Version:
9.2.x-dev
» 9.3.x-dev
Drupal 9.2.0-alpha1
will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the
Drupal core minor version schedule
and the
Allowed changes during the Drupal core release cycle
or
to post comments
Comment
#80
nikitagupta
commented
12 May 2021 at 05:43
Status:
Needs work
» Needs review
Status
File
Size
new
3015812-80.patch
35.56 KB
or
to post comments
Comment
#81
daffie
commented
9 July 2021 at 14:21
Status:
Needs review
» Needs work
The testbot is not happy.
or
to post comments
Comment
#82
jofitz
he/him
commented
3 August 2021 at 20:39
Status:
Needs work
» Needs review
Issue tags:
Needs reroll
Status
File
Size
new
3015812-82.patch
35.7 KB
Let's start by re-rolling the patch from
#80
...
or
to post comments
Comment
#83
daffie
commented
4 August 2021 at 09:22
Status:
Needs review
» Needs work
The patch is failing the testbot. See:
or
to post comments
Comment
#84
4 August 2021 at 09:22
Version:
9.3.x-dev
» 9.4.x-dev
Drupal 9.3.0-rc1
was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the
Drupal core minor version schedule
and the
Allowed changes during the Drupal core release cycle
or
to post comments
Comment
#85
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
29 January 2022 at 14:19
Issue tags:
LutskGCW22
Status
File
Size
new
3015812-85.patch
36.44 KB
new
interdiff-3015812-82-85.txt
0 bytes
Rerolled
#82
No interdiff.
or
to post comments
Comment
#86
andypost
he/him
Russian
commented
29 January 2022 at 14:42
Issue tags:
ContributionWeekend2022
7 files were hidden/shown/deleted
Status
File
Size
hidden
46-48-interdiff.txt
4.75 KB
hidden
3015812-2-48.patch
42.2 KB
hidden
3015812-72.patch
37.45 KB
hidden
3015812-77.patch
35.65 KB
hidden
3015812-80.patch
35.56 KB
hidden
3015812-82.patch
35.7 KB
hidden
interdiff-3015812-82-85.txt
0 bytes
re-roll went wrong
or
to post comments
Comment
#87
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
1 February 2022 at 22:33
Status
File
Size
new
3015812-87.patch
37.24 KB
1 file was hidden/shown/deleted
Status
File
Size
hidden
3015812-85.patch
36.44 KB
Since
#63
patch lost some important parts, another rerolling based on
#63
or
to post comments
Comment
#88
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
1 February 2022 at 22:44
Status:
Needs work
» Needs review
Status
File
Size
new
3015812-88.patch
37.34 KB
new
interdiff-3015812-87-88.txt
1.06 KB
1 file was hidden/shown/deleted
Status
File
Size
hidden
3015812-87.patch
37.24 KB
Fix CS
or
to post comments
Comment
#89
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
1 February 2022 at 22:53
Status
File
Size
new
3015812-89.patch
37.33 KB
new
interdiff-3015812-88-89.txt
1.33 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-88.patch
37.34 KB
hidden
interdiff-3015812-87-88.txt
1.06 KB
or
to post comments
Comment
#90
1 February 2022 at 23:49
Status:
Needs review
» Needs work
The last submitted patch,
89: 3015812-89.patch
, failed testing.
View results
or
to post comments
Comment
#91
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
2 February 2022 at 23:03
Status:
Needs work
» Needs review
Status
File
Size
new
3015812-91.patch
42.17 KB
new
interdiff-3015812-89-91.txt
7.07 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-89.patch
37.33 KB
hidden
interdiff-3015812-88-89.txt
1.33 KB
Fixed missing parts
or
to post comments
Comment
#92
alexpott
he/they
🇪🇺🌍
commented
3 February 2022 at 09:16
I always liked this patch. Really nice to see it moving again. Thanks @voleger. Now we're targeting Drupal 9 we can use return typehints and scalar typehints.
+++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
@@ -542,25 +542,27 @@ public function getPath($extension_name) {
+ protected function decorateExtension(Extension $extension) {
Can add a return typehint.
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+ public function __construct($root, $pathname, $filename, array $info, $status) {
Can add scalar typehints
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+ public function listAllRegions() {
...
+ public function listVisibleRegions() {
...
+ public function getDefaultRegion() {
Can add return typehint
+++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
@@ -262,29 +241,10 @@ protected function doGetBaseThemes(array $themes, $theme, array $used_themes = [
+ protected function decorateExtension(Extension $extension) {
Will need a return typehint.
or
to post comments
Comment
#93
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
3 February 2022 at 11:03
Status
File
Size
new
3015812-93.patch
42.24 KB
new
interdiff-3015812-91-93.txt
2.96 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-91.patch
42.17 KB
hidden
interdiff-3015812-89-91.txt
7.07 KB
Addressed
#92
or
to post comments
Comment
#94
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
3 February 2022 at 11:14
Status
File
Size
new
3015812-94.patch
42.25 KB
new
interdiff-3015812-93-94.txt
1.78 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-93.patch
42.24 KB
hidden
interdiff-3015812-91-93.txt
2.96 KB
Moved optional construct argument into the end of the list
or
to post comments
Comment
#95
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
3 February 2022 at 15:17
Tests passed. Ready for the review
or
to post comments
Comment
#96
andypost
he/him
Russian
commented
9 February 2022 at 10:24
Status:
Needs review
» Needs work
Related issues:
#2941155: ModuleHandler should not maintain list of installed modules now that ModuleExtensionList exists
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+ * Gets a list of all regions for the theme.
...
+ public function listAllRegions(): array {
s/Gets/Returns
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+ * Gets a list of visible regions for the theme.
...
+ public function listVisibleRegions(): array {
s/Gets/Returns
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+ * Gets the name of the default region for the theme.
s/Gets/Returns
+++ b/core/modules/block/block.module
@@ -123,8 +123,10 @@ function block_theme_initialize($theme) {
+ /** @var \Drupal\Core\Extension\Theme $theme_extension */
+ $theme_extension = \Drupal::service('theme_handler')->getTheme($theme);
// Apply only to new theme's visible regions.
- $regions = system_region_list($theme, REGIONS_VISIBLE);
+ $regions = $theme_extension->listVisibleRegions();
@@ -166,9 +168,10 @@ function block_modules_installed($modules) {
- foreach (\Drupal::service('theme_handler')->listInfo() as $theme => $data) {
- if ($data->status) {
...
+ foreach (\Drupal::service('theme_handler')->listInfo() as $theme => $theme_extension) {
+ if ($theme_extension->status) {
+ $regions = $theme_extension->listAllRegions();
+++ b/core/modules/block/src/BlockForm.php
@@ -186,7 +186,7 @@ public function form(array $form, FormStateInterface $form_state) {
- '#options' => system_region_list($theme, REGIONS_VISIBLE),
+ '#options' => $this->themeHandler->getTheme($theme)->listVisibleRegions(),
@@ -198,7 +198,7 @@ public function form(array $form, FormStateInterface $form_state) {
- $form['region']['#options'] = system_region_list($form_state->getValue('theme'), REGIONS_VISIBLE);
+ $form['region']['#options'] = $this->themeHandler->getTheme($form_state->getValue('theme'))->listVisibleRegions();
+++ b/core/modules/block/src/BlockListBuilder.php
@@ -197,7 +209,7 @@ protected function buildBlocksForm() {
- $regions = $this->systemRegionList($this->getThemeName(), REGIONS_VISIBLE);
+ $regions = $this->themeHandler->getTheme($this->getThemeName())->listVisibleRegions();
+++ b/core/modules/block/src/Controller/BlockController.php
@@ -110,9 +110,15 @@ public function demo($theme) {
+ return $this->themeHandler->getTheme($theme)->listVisibleRegions();
+++ b/core/modules/block/src/Entity/Block.php
@@ -346,10 +347,20 @@ public function preSave(EntityStorageInterface $storage) {
+ $theme_extension = \Drupal::service('theme_handler')->getTheme($this->theme);
wondering why theme handler used to access extension instead of
\Drupal\Core\Extension\ExtensionList::get()
, it may not need decoration and could use built-in caching. It reminds me abou
#2941155: ModuleHandler should not maintain list of installed modules now that ModuleExtensionList exists
+++ b/core/modules/block/src/Controller/BlockController.php
@@ -110,9 +110,15 @@ public function demo($theme) {
+ * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use
+ * $this->themeHandler->getTheme()->listVisibleRegions() instead.
...
+ @trigger_error(__CLASS__ . '::getVisibleRegionNames() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use $this->themeHandler->getTheme()->listVisibleRegions() instead. See https://www.drupal.org/node/3015925', E_USER_DEPRECATED);
should use 9.4.0, not clear why 8.8.0 used
or
to post comments
Comment
#97
andypost
he/him
Russian
commented
9 February 2022 at 10:47
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+ * Whether the theme is installed or not.
...
+ * @var int
...
+ public $status;
...
+ * The info array based on the theme's .info.yml file.
...
+ * @var array
...
+ public $info = [
+ 'engine' => 'twig',
+ 'regions' => [
+ 'sidebar_first' => 'Left sidebar',
+ 'sidebar_second' => 'Right sidebar',
+ 'content' => 'Content',
+ 'header' => 'Header',
+ 'primary_menu' => 'Primary menu',
+ 'secondary_menu' => 'Secondary menu',
+ 'footer' => 'Footer',
+ 'highlighted' => 'Highlighted',
+ 'help' => 'Help',
+ 'page_top' => 'Page top',
+ 'page_bottom' => 'Page bottom',
+ 'breadcrumb' => 'Breadcrumb',
...
+ * The relative path to the theme engine extension file.
...
+ * @var string
...
+ public $owner;
...
+ * The internal name of the theme engine extension.
...
+ * @var string
...
+ public $prefix;
not sure this properties should remain public
or
to post comments
Comment
#98
alexpott
he/they
🇪🇺🌍
commented
9 February 2022 at 11:18
Re
#97
yes they should - we have to address this in a follow-up to deprecate public access (or make them read-only). The issue here is that they are already in use like this. So we shouldn't change that here.
Re
#96
.4 and .5 - that's this patch showing it's age :)
or
to post comments
Comment
#99
alexpott
he/they
🇪🇺🌍
commented
9 February 2022 at 11:20
Also...
Gets a list of all regions for the theme.
could be
Lists all the theme's regions.
And
Gets a list of visible regions for the theme.
could be
Lists all the theme's visible regions.
Gets and Returns seem unnecessary... we can use the verb from the method name.
or
to post comments
Comment
#100
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
9 February 2022 at 13:06
Status
File
Size
new
3015812-100.patch
42.24 KB
new
interdiff-3015812-94-100.txt
1.96 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-94.patch
42.25 KB
hidden
interdiff-3015812-93-94.txt
1.78 KB
Addressed
#96
.1 .2 .5
or
to post comments
Comment
#101
sourabhjain
commented
22 February 2022 at 10:08
Status:
Needs work
» Needs review
or
to post comments
Comment
#102
22 February 2022 at 10:08
Version:
9.4.x-dev
» 9.5.x-dev
Drupal 9.4.0-alpha1
was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the
Drupal core minor version schedule
and the
Allowed changes during the Drupal core release cycle
or
to post comments
Comment
#103
andypost
he/him
Russian
commented
5 June 2022 at 16:13
Status
File
Size
new
interdiff.txt
11.75 KB
new
3015812-103.patch
41.81 KB
re-roll for 9.5
or
to post comments
Comment
#104
5 June 2022 at 17:17
Status:
Needs review
» Needs work
The last submitted patch,
103: 3015812-103.patch
, failed testing.
View results
or
to post comments
Comment
#105
andypost
he/him
Russian
commented
5 June 2022 at 17:50
the failed test does not work with stark somehow but classy is missing
or
to post comments
Comment
#106
catch
he/him
commented
6 June 2022 at 10:32
Version:
9.5.x-dev
» 10.0.x-dev
+++ b/core/modules/system/system.module
@@ -877,25 +888,35 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
* @return
* An array of regions in the form $region['name'] = 'description'.
+ *
+ * @deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Use
+ * \Drupal::service('theme_handler')->getTheme()->listAllRegions() or
+ * \Drupal::service('theme_handler')->getTheme()->listVisibleRegions()
+ * instead.
+ *
+ * @see https://www.drupal.org/node/3015925
*/
function system_region_list($theme, $show = REGIONS_ALL) {
- if (!$theme instanceof Extension) {
- $themes = \Drupal::service('theme_handler')->listInfo();
9.5 is closed to new code deprecations (modules and themes are still fair game) to give contrib a stable API to port against.
So I think this needs to be targeted at 10.1.x now.
Also I think this is probably more in 'major task' territory than 'major bug', seems like the 'bug' aspect of this is just a docs issue.
or
to post comments
Comment
#107
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
4 July 2022 at 18:59
Version:
10.0.x-dev
» 10.1.x-dev
Category:
Bug report
» Task
Status:
Needs work
» Needs review
Status
File
Size
new
3015812-107.patch
41.9 KB
new
interdiff-3015812-103-107.txt
10.92 KB
4 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-100.patch
42.24 KB
hidden
interdiff-3015812-94-100.txt
1.96 KB
hidden
interdiff.txt
11.75 KB
hidden
3015812-103.patch
41.81 KB
rerolled for drupal:10.1.x
or
to post comments
Comment
#108
smustgrave
commented
4 August 2022 at 16:11
Status
File
Size
new
interdiff-107-108.txt
2.04 KB
new
3015812-108.patch
41.91 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-107.patch
41.9 KB
hidden
interdiff-3015812-103-107.txt
10.92 KB
Rerolling so I can work on
or
to post comments
Comment
#109
andypost
he/him
Russian
commented
4 August 2022 at 21:31
systemRegionList($theme
should be deprecated as well
or
to post comments
Comment
#110
anybody
German
Porta Westfalica
commented
14 November 2022 at 12:09
Status:
Needs review
» Needs work
NW as of
#109
or
to post comments
Comment
#111
smustgrave
commented
14 November 2022 at 15:09
Status:
Needs work
» Needs review
Status
File
Size
new
interdiff-108-111.txt
1.55 KB
new
3015812-111.patch
41.86 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
interdiff-107-108.txt
2.04 KB
hidden
3015812-108.patch
41.91 KB
Rerolled but for #109 can you provide more detail? I see that systemRegionList is deprecated.
or
to post comments
Comment
#112
andypost
he/him
Russian
commented
14 November 2022 at 16:42
Status:
Needs review
» Needs work
+++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
@@ -542,25 +542,27 @@ public function getPath($extension_name) {
// Add the info file modification time, so it becomes available for
// contributed extensions to use for ordering extension lists.
$info['mtime'] = $extension->getMTime();
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+ $this->info['mtime'] = $this->getMTime();
somehow phpstan giving false positive here as the method executed via magic
\Drupal\Core\Extension\Extension::__call()
/**
* Re-routes method calls to SplFileInfo.
* Offers all SplFileInfo methods to consumers; e.g., $extension->getMTime().
*/
public function __call($method, array $args) {
if (!isset($this->splFileInfo)) {
$this->splFileInfo = new \SplFileInfo($this->root . '/' . $this->pathname);
return call_user_func_array([$this->splFileInfo, $method], $args);
or
to post comments
Comment
#113
smustgrave
commented
14 November 2022 at 17:01
So this a bug with something else?
or
to post comments
Comment
#114
andypost
he/him
Russian
commented
14 November 2022 at 18:21
Assigned:
Unassigned
mac_weber
Status:
Needs work
» Needs review
Status
File
Size
new
interdiff.txt
502 bytes
new
3015812-114.patch
42.35 KB
Let's see if this will allow to pass, thanks to @mglaman
or
to post comments
Comment
#115
andypost
he/him
Russian
commented
14 November 2022 at 18:23
Assigned:
mac_weber
» Unassigned
Status
File
Size
new
interdiff.txt
502 bytes
Sorry for assigning, the ref is
or
to post comments
Comment
#116
andypost
he/him
Russian
commented
14 November 2022 at 20:00
Status
File
Size
new
interdiff.txt
539 bytes
new
3015812-117.patch
42.35 KB
4 files were hidden/shown/deleted
Status
File
Size
hidden
interdiff-108-111.txt
1.55 KB
hidden
3015812-111.patch
41.86 KB
hidden
interdiff.txt
502 bytes
hidden
3015812-114.patch
42.35 KB
Fix CS
or
to post comments
Comment
#117
14 November 2022 at 21:10
Status:
Needs review
» Needs work
The last submitted patch,
116: 3015812-117.patch
, failed testing.
View results
or
to post comments
Comment
#118
smustgrave
commented
14 November 2022 at 21:54
Status:
Needs work
» Needs review
Status
File
Size
new
interdiff-116-118.txt
1.02 KB
new
3015812-118.patch
41 KB
3 files were hidden/shown/deleted
Status
File
Size
hidden
interdiff.txt
502 bytes
hidden
interdiff.txt
539 bytes
hidden
3015812-117.patch
42.35 KB
Fixed failing test.
or
to post comments
Comment
#119
andypost
he/him
Russian
commented
15 November 2022 at 02:38
Status:
Needs review
» Reviewed & tested by the community
I think it's ready for RM review
+++ b/core/lib/Drupal/Core/Extension/Extension.php
@@ -8,6 +8,8 @@
+ * @method int|false getMTime()
There's issue for that
#2959989: Deprecate Extension::__call() magic
or
to post comments
Comment
#120
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
15 November 2022 at 08:17
Status
File
Size
new
3015812-120.patch
44.93 KB
new
interdiff-3015812-118-120.txt
5.75 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
interdiff-116-118.txt
1.02 KB
hidden
3015812-118.patch
41 KB
Added deprecation tests for protected methods from the Block module
or
to post comments
Comment
#121
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
15 November 2022 at 08:57
Status
File
Size
new
3015812-121.patch
44.93 KB
new
interdiff-3015812-120-121.txt
1.59 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-120.patch
44.93 KB
hidden
interdiff-3015812-118-120.txt
5.75 KB
Fixed the typo in the test method name
or
to post comments
Comment
#122
quietone
commented
21 November 2022 at 04:20
Status:
Reviewed & tested by the community
» Needs work
Issue tags:
Needs change record updates
Sorry folks, the CR has an @todo. Can someone attend to that?
And I think the deprecation notices for the constants needs to be changed.
+++ b/core/modules/block/src/BlockRepositoryInterface.php
@@ -7,14 +7,25 @@ interface BlockRepositoryInterface {
+ * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0.
+ * It will not exist in Drupal 11, use
+ * \Drupal\Core\Extension\Theme::listVisibleRegions()
+ * instead.
+ *
+ * @see https://www.drupal.org/node/3015925
+ * @see \Drupal\Core\Extension\Theme::listVisibleRegions()
...
+ * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0.
+ * It will not exist in Drupal 11, use
+ * \Drupal\Core\Extension\Theme::listAllRegions() instead.
+ *
+ * @see https://www.drupal.org/node/3015925
+ * @see \Drupal\Core\Extension\Theme::listAllRegions()
While I do not see a specific case for deprecating constants, I also see no reason that this can't be same as for
deprecating method parameters
. That would mean the second @see in each case is removed as is "it will not exist in Drupal 1".
or
to post comments
Comment
#123
smustgrave
commented
21 November 2022 at 16:46
Status:
Needs work
» Needs review
Issue tags:
Needs change record updates
Status
File
Size
new
interdiff-121-123.txt
2.42 KB
new
3015812-123.patch
44.54 KB
2 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-121.patch
44.93 KB
hidden
interdiff-3015812-120-121.txt
1.59 KB
Took the description of this ticket for the CR.
Updated the comments too based on #122
or
to post comments
Comment
#124
andypost
he/him
Russian
commented
22 November 2022 at 10:47
Status:
Needs review
» Needs work
+++ b/core/lib/Drupal/Core/Extension/Extension.php
@@ -8,6 +8,8 @@
+ * @method int|false getMTime()
could be removed as already commited
#2959989: Deprecate Extension::__call() magic
+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+ $this->info['mtime'] = $this->getMTime();
needs fix
or
to post comments
Comment
#125
akram khan
Cuttack, Odisha
commented
22 November 2022 at 19:41
Status
File
Size
new
3015812-125.patch
32.66 KB
new
reroll_diff_123-125.txt
22.54 KB
Updated Patch and address #124
or
to post comments
Comment
#126
akram khan
Cuttack, Odisha
commented
22 November 2022 at 20:15
Status
File
Size
new
Screenshot 2022-11-23 at 1.42.21 AM.png
73.35 KB
Sorry for added wrong in #125
when applied patch #123 on 10.1.x it throwing some error added screen shot as well
or
to post comments
Comment
#127
viappidu
commented
23 November 2022 at 00:59
Status:
Needs work
» Needs review
Status
File
Size
new
3015812-127.patch
44.08 KB
1 file was hidden/shown/deleted
Status
File
Size
hidden
3015812-125.patch
32.66 KB
Worked 3015812-123.patch addressing comments on #124
Main difference (after
#2959989: Deprecate Extension::__call() magic
):
$this->info['mtime'] = $this->getMTime();
Becomes
$this->info['mtime'] = $this->getFileInfo()->getMTime();
or
to post comments
Comment
#128
andypost
he/him
Russian
commented
23 November 2022 at 12:01
Status:
Needs review
» Reviewed & tested by the community
4 files were hidden/shown/deleted
Status
File
Size
hidden
interdiff-121-123.txt
2.42 KB
hidden
3015812-123.patch
44.54 KB
hidden
reroll_diff_123-125.txt
22.54 KB
hidden
Screenshot 2022-11-23 at 1.42.21 AM.png
73.35 KB
Checked last patch and looks like it's ready
or
to post comments
Comment
#129
quietone
commented
26 December 2022 at 02:28
Status:
Reviewed & tested by the community
» Needs work
Sorry folks. The patch is fail commit code checks.
or
to post comments
Comment
#130
ameymudras
commented
26 December 2022 at 11:16
Status
File
Size
new
3015812-130.patch
43.97 KB
#127 was not getting applied, I have made changes and hopefully, it should work. Couldn't generate the interdiff here because of "Whitespace damage detected in input"
or
to post comments
Comment
#131
ameymudras
commented
26 December 2022 at 11:17
Status:
Needs work
» Needs review
or
to post comments
Comment
#132
quietone
commented
11 January 2023 at 20:28
Issue tags:
Needs release manager review
This is tagged for a release manager review. In #67 a question was asked regarding the compatibility of the deprecation being added with the one added in the previous minor version. At that time that this was adding a deprecation to 8.9. Those concerns are no longer relevant because there isn't a supported version of Drupal with those deprecations. I discussed this issue with xjm and we agree.
I am removing the 'Needs release manager review' tag.
or
to post comments
Comment
#133
needs-review-queue-bot
commented
30 January 2023 at 23:51
Status:
Needs review
» Needs work
Status
File
Size
new
3015812-nr-bot.txt
147 bytes
The
Needs Review Queue Bot
tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the
Drupal Contributor Guide
to find step-by-step guides for working with issues.
or
to post comments
Comment
#134
30 January 2023 at 23:51
Version:
10.1.x-dev
» 11.x-dev
Drupal core is moving towards using a “main” branch.
As an interim step, a new
11.x
branch has been opened
, as Drupal.org infrastructure cannot currently fully support a branch named
main
. New developments and disruptive changes should now be targeted for the
11.x
branch, which currently accepts only minor-version allowed changes. For more information, see the
Drupal core minor version schedule
and the
Allowed changes during the Drupal core release cycle
or
to post comments
Comment
#135
voleger
he/his
Ukrainian
Ukraine, Rivne
commented
27 November 2024 at 16:10
3 files were hidden/shown/deleted
Status
File
Size
hidden
3015812-127.patch
44.08 KB
hidden
3015812-130.patch
43.97 KB
hidden
3015812-nr-bot.txt
147 bytes
Introduced MR which contains
#130
patch.
or
to post comments
Comment
#136
27 November 2024 at 16:11
voleger
opened
merge request !10369
or
to post comments
Comment
#137
andypost
he/him
Russian
commented
2 May 2025 at 11:58
Related issues:
#2479459: BlockRepository::getVisibleBlocksPerRegion() does an uncached entity query on every request
, +
#2538970: Improve the speed of \Drupal\Core\Theme\ThemeAccessCheck
or
to post comments
Comment
#138
andypost
he/him
Russian
commented
2 May 2025 at 12:43
btw when core will require PHP 8.5 we can deprecate constants
or
to post comments
Comment
#139
nicxvan
commented
23 June 2025 at 03:22
Component:
system.module
» extension system
or
to post comments
Comment
#140
xjm
she/her
commented
27 June 2025 at 17:19
For the RM review mentioned in
#132
or
to post comments
Comment
#141
27 June 2025 at 17:19
Version:
11.x-dev
» main
Drupal core is now using the
main
branch as the primary development branch. New developments and disruptive changes should now be targeted to the
main
branch.
Read more in the announcement
or
to post comments
Comment
#142
berdir
German
Switzerland
commented
7 February 2026 at 11:13
Status:
Needs work
» Needs review
Updated and rebased, still running tests, but I think in a state where it can be reviewed.
or
to post comments
Comment
#143
claudiu.cristea
Romanian
Arad 🇷🇴
commented
7 February 2026 at 11:25
Status:
Needs review
» Needs work
I think the const replacing enum is missing
or
to post comments
Comment
#144
berdir
German
Switzerland
commented
7 February 2026 at 12:09
Status:
Needs work
» Needs review
Enum is not missing. Issue summary has an overview of all the changes here. Still expecting some test fails, but the worst one should hopefully be fixed now.
or
to post comments
Comment
#145
nicxvan
commented
7 February 2026 at 13:46
Got some comments on the MR, I need to think about this approach a bit,
It is nice to remove the need entirely of these constants.
In not sure about the decorating pattern, but it seems to make sense.
or
to post comments
Comment
#146
berdir
German
Switzerland
commented
8 February 2026 at 09:32
Status:
Needs review
» Needs work
I think the main remaining thing for me is the decorate naming/concept. This has been discussed quite a bit already, in #46/#47 for example. I don't think this is a decorator. Yes, a decorator does return a new object, but it's IMHO one that adheres to the original interface. That's not the case there. Theme is a child class that adds its own methods and concepts that do not exist on the parent. Decorator specifically allows to decorate the same object multiple times.
@dawehner in #47 also talks that we use composition and not inheritance, but that's not true, neither then nor now. Theme extends Extension, so it is inheritance. We're working with value objects and not interfaces, as far as I see it's impossible to use composition here if we want Theme objects to be used as more generic Extension objects too.
The only idea I have right now is to use a less opinionated, more generic term for this method, possibly just alterExtension(), which would kind of match the system_info_alter hook. Possibly wrap, *if* it were actually composition. Is there a verb that we could use that translates to "Use specific subclass"?
I'm also wondering if we should split the info parsing from the altering/subclassing. I'm not really convinced of the move of the defaults and mtime from ThemeExtensionList to the Theme constructor. I think we could keep it where it is and then we have to change fewer things. This started before ThemeExtensionList existed, so the context here changed quite a bit.
I think my proposal would be something like this:
foreach ($extensions as $extension) {
$extension->info = $this->createExtensionInfo($extension);
$extension = $this->alterExtension($extension);
I thought about using $info and passing that around as a second argument, but that's no longer an alter then and it's doing multiple things again.
Speaking of mtime, I was confused about that one too, because I don't see any usage of this in core outside of tests. This was added 14 years ago in
#1355526: Add a way to determine the date a module was added so the modules page can use it for sort
, even then it was just verified in tests. I'd assume that mtime is rarely useful with composer and modern deployment processes. And package manager and stuff is going to deal with "enable a recently enabled module" in its own way.
or
to post comments
Comment
#147
berdir
German
Switzerland
commented
10 February 2026 at 23:01
Status:
Needs work
» Needs review
I implemented my suggested change now, essentially restoring createExtensionInfo() and introducing subClassExtension() per suggestion from @alexpott This significantly simplifies the required changes in the Extension component.
There is more that could be done in ThemeExtensionList. This moves the status and only the status from doList() to subClassExtension() as that was in decorate before as well. We could:
a) further reduce the required changes here by restoring how status is set now and deal with that later.
b) keep it as is.
c) Move more logic from doList and set them through the constructor/methods. There are still several dynamic properties being set there, like module_dependencies, base_themes and sub_themes. But we can't move everything as parts depend on having all themes and then adding that information.
I think c) is a rabbit hole that will blow this up considerably. I don't care much between a) or b). What I would suggest is that we mark Theme as either @final or real final, to make it clear that we do not support subclassing this. We want to change the constructor, add methods and properties later without having to worry about subclasses.
There are possibly even more options. We could do the subclass in doList() and not introduce subClassExtension() at all, then the parent would not need any changes, but I think this is fine as an extension point for other extension types later on.
or
to post comments
Comment
#148
nicxvan
commented
11 February 2026 at 02:48
Yeah a or b make sense.
To be honest this feels manageable at this point so leaving it as is is probably fine.
If we want to be extra careful rolling that bit back it's the way to go.
@final makes sense too, I'm strongly against real final.
or
to post comments
Comment
#149
alexpott
he/they
🇪🇺🌍
commented
11 February 2026 at 11:00
Re
#147
I guess reading that and thinking about my comment about readonly - means that actually I'd be in favour of (a). Let's do a minimal change.
Also +1 to final or @final. I think in this case a real final might be better considering we know that this issue is the start of quite a few changes necessary to make the Theme class behave the way we'd like.
or
to post comments
Comment
#150
berdir
German
Switzerland
commented
11 February 2026 at 22:58
@alexpott, see updates, is that what you had in mind? I also added a @final, I agree with you, but I'm doing my best to avoid any holy wars around final, either is fine for me.
or
to post comments
Comment
#151
alexpott
he/they
🇪🇺🌍
commented
12 February 2026 at 09:29
This looks great and yeah avoiding holy wars seems like a good idea.
or
to post comments
Comment
#152
nicxvan
commented
20 February 2026 at 05:07
Status:
Needs review
» Reviewed & tested by the community
I think this is ready for the next steps, I took another in depth look at the code, and it's come together very nicely!
I also edited the CR pretty heavily, it was targeted towards 8.7!!!
or
to post comments
Comment
#153
godotislate
he/him
commented
12 March 2026 at 17:48
Status:
Reviewed & tested by the community
» Needs work
NW for merge conflict.
or
to post comments
Comment
#154
nicxvan
commented
12 March 2026 at 18:55
Status:
Needs work
» Reviewed & tested by the community
Rebased, it was a comment on a deprecated method that was removed in the deprecation removal bunch of issues.
or
to post comments
Comment
#155
godotislate
he/him
commented
8 April 2026 at 22:34
Status:
Reviewed & tested by the community
» Needs work
The phpstan baseline needs a rebase. I also added comments to the MR.
Also, I know the "decorate" or "subclassExtension" terminology has already been much discussed, so I don't want to block this, but I have an alternate idea that could be done in a follow up if people think it's worthwhile. Instead of copying an Extension object to a subclass object, we could do this:
Add an optional "
$extensionClass
" constructor parameter and class property to ExtensionDiscovery, so that it looks like this:
public function __construct(string $root, $use_info_parser = TRUE, ?array $profile_directories = NULL, ?string $site_path = NULL, protected string $extensionClass = Extension::class) {
$this->root = $root;
$this->profileDirectories = $profile_directories;
$this->sitePath = $site_path;
$this->infoParser = $use_info_parser ? new InfoParser($root) : NULL;
And in ExtensionDiscovery::scanDirectory() change
new Extension($this->root, $type, $pathname, $filename)
to
new $this->extensionClass($this->root, $type, $pathname, $filename)
The constructor of Theme would need to be changed to match Extension, so there'd be some brittleness there. The alternative would be to put it in a createExtension() method, but then every ExtensionList would need a corresponding discovery class with createExtension overridden.
In
ThemeExtensionList::getExtensionDiscovery()
protected function getExtensionDiscovery() {
return new ExtensionDiscovery($this->root, extensionClass: Theme::class);
or
to post comments
Comment
#156
nicxvan
commented
9 April 2026 at 02:06
Thank you for the review!
I reviewed all of your suggestions they look great. I will hold off on applying them to preserve my ability to rtbc.
In reviewing your suggestions I noticed there are two properties we no longer need in the theme extension object as well.
Happy to create a follow up for: 155.
or
to post comments
Comment
#157
berdir
German
Switzerland
commented
9 April 2026 at 06:35
I think the main concern I have with #155 and pushing the Theme object down into discovery is that we'd need to forever support partial theme objects. All the theme stuff that depends on the .info.yml content would need to be optional/nullable and then added during a later build phase, so we'd need to keep track of partial/finalized objects or something like that. If not for that I'd be fine exploring this also in this issue, but a follow-up to try that seems fine.
or
to post comments
Comment
#158
godotislate
he/him
commented
10 April 2026 at 16:33
Discussed with @nicxvan: prefix and owner properties on Theme should be deprecated here and then removed in a follow up for D12.
Consulting with other release managers about the deprecated constants and protected methods.
or
to post comments
Comment
#159
berdir
German
Switzerland
commented
10 April 2026 at 19:34
Status:
Needs work
» Needs review
As discussed on slack, removed two properties, I realized that extension objects allow dynamic properties and we have plenty of those, so it's fine to remove them, should not cause any issues in 11.x, although we might want to do do a 11.x MR just to be certain before merging.
The remaining thing then is the deprecated controller/form method and how to handle that.
or
to post comments
Comment
#160
nicxvan
commented
12 April 2026 at 16:54
I reviewed this, looks great. I'm not going to mark it since we are waiting on the protected method question.
or
to post comments
Comment
#161
godotislate
he/him
commented
13 April 2026 at 05:48
Discussed with @catch and @xjm, and the general sentiment is that we care a lot about removing usage of deprecated code paths when they are being deprecated, to help make sure that they actually can be deprecated.
So I gave this issue some more though in light of that. Since protected methods are not part of our BC promise, I looked at whether we could just remove them directly in 11.4. In contrib, there is of
BlockListBuilder::systemRegionList()
), so to minimize disruption, I think it's fine to go with deprecate the methods. But I still think it's best to remove the usage of the
system_region_list()
and the constants in those methods, and it's consistent with the change to
BlockController::getVisibleRegionNames()
anyway. I also took a quick peek at
#1452100: Private file download returns access denied, when file attached to revision other than current
, and compared to there, removing the deprecated function usage is straightforward enough.
or
to post comments
Comment
#162
berdir
German
Switzerland
commented
14 April 2026 at 09:23
Accepted the two suggestions for the implementation of the method, but not the constant changes, did I understand you correctly with that?
> and compared to there, removing the deprecated function usage is straightforward enough.
It certainly is. I'm happy as long as we don't imply that doing it here also means we must do it there and evaluate that separately considering the complexity there.
or
to post comments
Comment
#163
godotislate
he/him
commented
14 April 2026 at 21:36
It's fine to go forward without the constant changes.
or
to post comments
Comment
#164
nicxvan
commented
14 April 2026 at 21:42
Status:
Needs review
» Reviewed & tested by the community
In that case I think this is ready again!
or
to post comments
Comment
#165
20 April 2026 at 20:50
godotislate
committed
b30fe98d
on
main
feat: #3015812 Introduce new Theme extension object and properly...
or
to post comments
Comment
#166
20 April 2026 at 20:51
godotislate
closed
merge request !10369
or
to post comments
Comment
#167
godotislate
he/him
commented
20 April 2026 at 20:55
Status:
Reviewed & tested by the community
» Patch (to be ported)
Thanks to everyone who helped on this one over. Did my best to update credit.
Committed
b30fe98
and pushed to main. Thanks!
There's a merge conflict on block.module for 11.x, so moving to Patch to be ported for that.
Also just noticed that the URLs for the CR for the constant deprecations in system.module are wrong. Will push a fix for that shortly. Please make sure to include those changes in the 11.x port.
or
to post comments
Comment
#168
20 April 2026 at 21:02
godotislate
committed
1ab4bfda
on
main
refactor: #3015812 Introduce new Theme extension object and properly...
or
to post comments
Comment
#169
20 April 2026 at 21:44
berdir
opened
merge request !15506
or
to post comments
Comment
#170
berdir
German
Switzerland
commented
20 April 2026 at 21:45
Status:
Patch (to be ported)
» Needs review
Created a new 11.x MR, including the follow-up.
Had to fix/reintroduce a few additional changes that aren't in main. The migrate tests need the fix for new BlockHooks(), and deprecatation test method in core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php needed to be adjusted as well.
or
to post comments
Comment
#171
needs-review-queue-bot
commented
21 April 2026 at 05:34
Status:
Needs review
» Needs work
Status
File
Size
new
3015812-nr-bot_damzgl84.txt
91 bytes
The
Needs Review Queue Bot
tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the
Drupal Contributor Guide
to find step-by-step guides for working with issues.
or
to post comments
Comment
#172
berdir
German
Switzerland
commented
21 April 2026 at 05:48
Status:
Needs work
» Needs review
Issue tags:
no-needs-review-bot
bot is confused about the 11.x branch I think.
or
to post comments
Comment
#173
needs-review-queue-bot
commented
21 April 2026 at 12:37
Status:
Needs review
» Needs work
Status
File
Size
new
3015812-nr-bot_e0ovh3_c.txt
91 bytes
The
Needs Review Queue Bot
tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the
Drupal Contributor Guide
to find step-by-step guides for working with issues.
or
to post comments
Comment
#174
smustgrave
commented
21 April 2026 at 12:41
Version:
main
» 11.x-dev
Status:
Needs work
» Reviewed & tested by the community
Surprised it ignored the tag
or
to post comments
Comment
#175
smustgrave
commented
21 April 2026 at 13:05
Status:
Reviewed & tested by the community
» Needs review
Meant review per the bot
or
to post comments
Comment
#176
nicxvan
commented
21 April 2026 at 13:14
Status:
Needs review
» Reviewed & tested by the community
Ok I reviewed this, pretty tedious.
Only differences were:
One additional case in ThemeHandlerTest needed updating to Theme.
3 or 4 Migrate tests needed fixing for the block rebuild call.
The tests are failing, but only committers have access to review this branch right now so please kick it back if it's a real failure.
or
to post comments
Comment
#177
nicxvan
commented
21 April 2026 at 13:17
Parent issue:
#3566536: [meta] eliminate core .module files
or
to post comments
Comment
#178
21 April 2026 at 16:37
godotislate
closed
merge request !15506
or
to post comments
Comment
#179
21 April 2026 at 16:37
godotislate
committed
0f4d171d
on
11.x
refactor: #3015812 Introduce new Theme extension object and properly...
or
to post comments
Comment
#180
godotislate
he/him
commented
21 April 2026 at 16:37
Status:
Reviewed & tested by the community
» Fixed
Committed
0f4d171
and pushed to 11.x. Thanks!
or
to post comments
Comment
#181
21 April 2026 at 16:37
Now that this issue is closed,
review the
contribution record
As a contributor, attribute any organization that helped you, or if you volunteered your own time.
Maintainers, credit people who helped resolve this issue.
or
to post comments
Contribution record
Change records for this issue
There is a new Theme extension object. system_region_list() and system_default_region() and region related constants are deprecated
Parent issue
Add child issue
clone issue
Related issues
Referenced by
#2024043: Add Module, Theme, Profile, and Extension value objects
#2603598: Improve default region order
#2635166: block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default
#2721603: ExtensionList cleanup and separation of concerns
#2940481: Road to stabilize ExtensionList services
#2999721: [META] Deprecate the legacy include files before Drupal 9
#3026232: Deprecate Theme extension object public properties
#3035288: Deprecate theme_get_setting()
#3097453: Remove system.module BC layers
#3108731: Remove deprecated REGIONS_ALL constant
#3111942: Remove all remaining @deprecated code from system module
#3571172: Deprecate two functions in system module
Infrastructure management for Drupal.org provided by
Need a Drupal 7 extended support partner? Consider Tag1.
News items
News
Planet Drupal
Social media
Sign up for Drupal news
Security advisories
Jobs
Our community
Community
Services
Training
Hosting
Contributor guide
Groups & meetups
DrupalCon
Code of conduct
Documentation
Documentation
Drupal Guide
Drupal User Guide
Developer docs
API.Drupal.org
Drupal code base
Download & Extend
Drupal core
Modules
Themes
Distributions
Governance of community
About
Web accessibility
Drupal Association
About Drupal.org
Drupal is a
registered trademark
of
Dries Buytaert