Problem/Motivation

The code in \Drupal\Core\Theme\ThemeInitialization::getActiveTheme() is incorrect and results in incorrect base theme info in the active theme.

Proposed resolution

Fix the line $base_active_themes[$base_theme->getName()] = $this->getActiveTheme($base_theme, array_slice($base_themes, 1)); so that it always creates the base theme with the correct base themes.

There'll also be a smaller cache entry.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#7 issue.png52.82 KBmahtab_alam
#2 3019482-2.patch2.17 KBalexpott
#2 3019482-2.test-only.patch1.1 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3019482-2.test-only.patch1.1 KB
new 3019482-2.patch2.17 KB

The last submitted patch, 2: 3019482-2.test-only.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented

Nice catch! Does it need extend test with a case when theme has more then one base theme?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@andypost we have that here - it's subsubtheme -> subtheme -> basetheme :)

But you are right that it is worse when you have something like thunder_admin -> seven -> classy -> stable because in that instance it looks like seven is a base theme of classy!!!!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#3020157: ActiveTheme objects and cache contain way too much information

FYI this patch is made obsolete by the changes in #3020157: ActiveTheme objects and cache contain way too much information - however this one is a bugfix and can be backported to 8.6.x

mahtab_alam’s picture

mahtab_alam commented
StatusFileSize
new issue.png52.82 KB

I have applied this patch on my local 8.7x-dev instance. But it failed
issue

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

I can not confirm that. For me the patch from #2 applies cleanly.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@mahtab_alam thank you for reviewing this issue!

The automated testing infrastructure tells us whether the patch applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

@mahtab_alam in light of the above I've removed your issue credit and can also confirm the patch still applies.

mahtab_alam’s picture

mahtab_alam commented

@alexpott Thanks for guiding me for the workflow on how we review the patch and other aspects related to contribution.
In future i will check the things before commenting out here.

mahtab_alam’s picture

mahtab_alam commented

@alexpott Thanks for guiding me for the workflow on how we review the patch and other aspects related to contribution.
In future i will check the things before commenting out here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Closed (duplicate)

Fixed in #3020157: ActiveTheme objects and cache contain way too much information