Problem/Motivation

Overriding \Drupal\Core\Theme\Registry is dangerous because it could break many assumptions made in the theme system. Theme registry contents should be frozen but the way it's generated shouldn't. We should also be able to add features to Registry in minor releases and this isn't really possible right now because unknown places (custom/contrib/core themes and modules) trust the way it works. This logic shouldn't be overrideable (or we should tell people to not do it) because it's connected very tightly to the whole theme system.

Why this is RC eligible

This only changes documentation by marking a class internal.

Proposed resolution

Mark \Drupal\Core\Theme\Registry internal.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 mark-2600228-12.patch685 byteslauriii
#8 interdiff.txt608 byteslauriii
#8 mark-2600228-8.patch659 byteslauriii
#6 interdiff.txt585 byteslauriii
#6 mark-2600228-6.patch656 byteslauriii
#2 mark-2600228-2.patch435 byteslauriii
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Status: Active » Needs review
FileSize
mark-2600228-2.patch435 bytes
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Twig

Looks good, added some rationale to the IS.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Assigned: Unassigned » alexpott
Issue tags: +rc eligible

This is RC eligible since it's just docs, but I'm not sure the rules around marking stuff @internal or not. And since this seems like a framework manager decision, assigning to alexpott, who I guess already talked to lauriii about this a bit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -18,6 +18,8 @@
+ * @internal
+ *

I think there should be an explanation.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Status: Needs work » Needs review
FileSize
mark-2600228-6.patch656 bytes
interdiff.txt585 bytes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -18,6 +18,10 @@
+ *   new features in minor releases so this class should be considered final.

The last word is a little bit odd. I would have expected 'internal' ...

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
FileSize
mark-2600228-8.patch659 bytes
interdiff.txt608 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Assigned: alexpott » Unassigned
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I think this is a good move to make this internal.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -18,6 +18,10 @@
+ * @internal Expected to be used only internally since every hook_theme()
+ *   implementation depends on the way this class is built. This class may get
+ *   new features in minor releases so this class should be considered internal.

According to @jhodgdon @internal should stand for itself. Additional information can be provided afterwards

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Status: Needs work » Needs review
FileSize
mark-2600228-12.patch685 bytes
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

This is RTBC as long as the request in #11 meant "Stand by itself" instead of "stand for itself"

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

This needs a short change record since we're doing it within a stable release now - just gives people less excuses if they actually did subclass this to complain later.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Created change record

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Has the CR, rational seems reasonable to me, back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 7acb0de and pushed to 8.1.x. Thanks!

  • alexpott committed 6020139 on 8.2.x 
    Issue #2600228 by lauriii: Mark \Drupal\Core\Theme\Registry internal

  • alexpott committed 7acb0de on 8.1.x 
    Issue #2600228 by lauriii: Mark \Drupal\Core\Theme\Registry internal

(...
fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm at OSInet commented

Is it not inconsistent to have a hook part of our public API ( hook_theme_registry_alter() ) access an @internal class ?

cilefen’s picture

cilefen CreditAttribution: cilefen commented

hook_theme_registry_alter is modifying a discovered array, not calling functions. No?

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm at OSInet commented

Indeed, but it contains a @see referencing this @internal structure.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

The @see is a reference to documentation explaining the array structure passed to hook_theme_registry_alter. I do not see how this could be a problem.

Status: Fixed » Closed (fixed)

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