Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#18 | theme_handler-2187717-18.patch | 14.19 KB | dawehner |
#15 | interdiff.txt | 6.76 KB | sun |
#15 | theme.base_.15.patch | 10.95 KB | sun |
#10 | interdiff.txt | 417 bytes | sun |
#10 | drupal8.theme-base-list.10.patch | 13.27 KB | sun |
Comments
Comment #1
sunThis 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.Comment #2
sunAn 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,
$theme->template
property is obsolete.Comment #3
sunNote 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
Comment #4
sunThis is obsolete and wrong, too.
Comment #8
sunWow, it would really help if advanced code logic was properly documented... :p
Comment #9
sunAnd, 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...
Comment #10
sunSorry, I missed a detail of the current base theme discovery process.
Comment #15
sunOK, I don't have the energy to investigate those test failures right now, so going back to #8:
Comment #16
sunFinally green — this legacy code cleanup blocks #1067408: Themes do not have an installation status
Comment #17
dawehnerWhat 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
Comment #18
dawehnerLet's also expand the test coverage about parents/sub themes.
Comment #19
sunYes,
$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.
Comment #20
xjm#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.
Comment #21
dawehnerThe last interdiff just expanded the test coverage.
Comment #22
webchickThis 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!