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

Comments

lauriii created an issue. See original summary.

lauriii’s picture

Status: Active » Needs review
FileSize
435 bytes
Cottser’s picture

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

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

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

Status: Needs work » Needs review
FileSize
656 bytes
585 bytes
dawehner’s picture

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

alexpott’s picture

Assigned: alexpott » Unassigned
alexpott’s picture

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

dawehner’s picture

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

Status: Needs work » Needs review
FileSize
685 bytes
joelpittet’s picture

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

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

Status: Needs work » Needs review
Issue tags: -Needs change record

Created change record

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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

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

cilefen’s picture

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

fgm’s picture

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

cilefen’s picture

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.