Updated: Comment #N

Problem/Motivation

Research what the parent/sub theme inheritance code in system_list() is actually doing, and why it is seemingly duplicated into ThemeHandler::rebuildThemeData()

Proposed resolution

Output of the research:

  • The logic is already mostly in the ThemeHandler
  • We can remove a bunch of as @deprecate marked APIs

Remaining tasks

User interface changes

API changes

Original report by @sun

CommentFileSizeAuthor
#18 theme_handler-2187717-18.patch14.19 KBdawehner
#15 interdiff.txt6.76 KBsun
#15 theme.base_.15.patch10.95 KBsun
#10 interdiff.txt417 bytessun
#10 drupal8.theme-base-list.10.patch13.27 KBsun
#9 interdiff.txt3.89 KBsun
#9 drupal8.theme-base-list.9.patch13.2 KBsun
#8 interdiff.txt1.57 KBsun
#8 drupal8.theme-base-list.8.patch10.11 KBsun
#4 interdiff.txt1.5 KBsun
#4 drupal8.theme-base-list.4.patch9.79 KBsun
#2 drupal8.theme-base-list.2.patch10.04 KBsun
#1 drupal8.theme-base-list.1.patch3.1 KBsun
system_list-vs-ThemeHandler-rebuildThemeData.png101.5 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
drupal8.theme-base-list.1.patch3.1 KB

This code really looks 90% identical to me...

But let me first try and see what happens if the existing code is moved into rebuildThemeData() only.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Assigned: Unassigned » sun
Related issues: +#1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files", +#1545964: Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme, +#1297856: Base themes should be required dependencies of subthemes
FileSize
drupal8.theme-base-list.2.patch10.04 KB

An extensive dig into archives yields my comment in #1033116-60: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files"

→ The theme engine 'theme' (pure PHP) is obsolete and no longer exists in D8.

Due to that,

  1. the $theme->template property is obsolete.
  2. every theme has to have an engine, but the default is 'twig'.
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Note that 95% of this patch are basically needless clean-ups — the one and only purpose was to provide sufficient diff context to allow this patch to be actually reviewed :P

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.theme-base-list.4.patch9.79 KB
interdiff.txt1.5 KB 
+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -271,61 +271,64 @@ public function rebuildThemeData() {
         else {
-          $themes[$key]->prefix = $key;
+          $sub_theme->prefix = $key;
         }

This is obsolete and wrong, too.

The last submitted patch, 2: drupal8.theme-base-list.2.patch, failed testing.

The last submitted patch, 2: drupal8.theme-base-list.2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: drupal8.theme-base-list.4.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.theme-base-list.8.patch10.11 KB
interdiff.txt1.57 KB

Wow, it would really help if advanced code logic was properly documented... :p

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.theme-base-list.9.patch13.2 KB
interdiff.txt3.89 KB

And, wow, why all of this insane complexity in the first place?

The error condition is not handled at all to begin with! — A sub-theme with a missing base theme is apparently made available, as if the sub-theme would not have declared a parent theme...

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.theme-base-list.10.patch13.27 KB
interdiff.txt417 bytes

Sorry, I missed a detail of the current base theme discovery process.

The last submitted patch, 8: drupal8.theme-base-list.8.patch, failed testing.

The last submitted patch, 9: drupal8.theme-base-list.9.patch, failed testing.

The last submitted patch, 9: drupal8.theme-base-list.9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: drupal8.theme-base-list.10.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
theme.base_.15.patch10.95 KB
interdiff.txt6.76 KB

OK, I don't have the energy to investigate those test failures right now, so going back to #8:

  1. Reverted sub/base-theme processing to #8.
  2. Fixed Drupal\Tests\Core\Extension\ThemeHandlerTest.
  3. Reverted sub/base theme process to not skip/remove unavailable themes.
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Finally green — this legacy code cleanup blocks #1067408: Themes do not have an installation status

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Priority: Major » Normal
Issue summary: View changes 
+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -271,62 +271,63 @@ public function rebuildThemeData() {
-        $themes[$key]->template = TRUE;

What about this line? Is it no not used at all?

@sun
Can you clarify whether the new issue summary is something you would agree with?

Just in case someone asks, there is a change notice already for the removed functions: https://drupal.org/node/2150863

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt3.42 KB
theme_handler-2187717-18.patch14.19 KB

Let's also expand the test coverage about parents/sub themes.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Yes, $theme->template = TRUE is a stale property and not used anymore.

The (PHP-only) "theme" theme engine no longer exists, which inherently means that every theme engine uses templates, which was the only purpose of this indicator.

#1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files" should have removed it, but did not.

To clarify: There are actually no actual changes in this patch. We're just removing some obsolete code.

As mentioned earlier, the one and only purpose for the technically irrelevant cleanup changes was to provide diff context.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Major

#1067408: Themes do not have an installation status is a beta blocker and was postponed on this, so let's keep this at major as a soft blocker for that issue.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +PHPUnit

The last interdiff just expanded the test coverage.

webchick’s picture

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

This all looks fine, apart from the hunk that removes oodles of useful documentation about what's in one of these theme info arrays, without putting it back anywhere. But I dug around in ThemeHandlerInterface, and the docs are duplicated above the listInfo() method, which hopefully eventually we'll be able to use straight-up versus the list_themes() wrapper, so this looks legit.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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