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.
If you have a theme that uses a theme engine other then PHPTemplate, it still gives you the option of using that theme. Since the theme's theme engine is not available, it should disable the theme, much like how the module system disables access to a module when a module dependency is not available.
Steps to reproduce:
- Install a theme that requires a theme engine other then PHPTemplate, like Bluemarine ETS
- Make sure you don't install the theme's dependent theme engine
- Visit admin/build/themes and see that you can still use the theme even though its required theme engine is not available
Comment | File | Size | Author |
---|---|---|---|
#79 | 304540-79.patch | 6.56 KB | therealssj |
#71 | 304540-66.patch | 3.09 KB | r.nabiullin |
Comments
Comment #1
dvessel CreditAttribution: dvessel commentedThemes without an engine can be created. See Chameleon.
Comment #2
dvessel CreditAttribution: dvessel commentedNever mind.. Misread, sorry.
Comment #3
RobLoachWhat other requirements are there for a theme to be enabled? With this patch, it just checks for the theme engine, the core compatibility, and the PHP version. Anything else? Maybe base themes?
Comment #4
dvessel CreditAttribution: dvessel commentedHere are all the .info options.
There's the "base theme". If that's defined but doesn't exist, shouldn't we have the same behavior?
Comment #5
RobLoachAdding the base theme as a requirement.
Comment #6
RobLoachThere we go.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #8
JohnAlbinSubscribing. This is a common problem with Zen sub-themes. “Oh. I need to install the Zen theme, too?” :-p
This should be backported to D6 too, IMO.
Comment #9
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedSubscribing. +1 for back-porting to D6.
Comment #10
jbrauer CreditAttribution: jbrauer commentedAnother component which may be listed elsewhere but I haven't seen is that the plugin manager happily downloads and installs a sub-theme leading to all sorts of errors if the base theme isn't already installed. Ideally this would be handled in a way similar to module dependencies.
Marking as major because the effect on site administrators will be significant.
Comment #11
marcingy CreditAttribution: marcingy commentedNew version of patch againsy current head that checks the engine and base theme presence.
Comment #12
marcingy CreditAttribution: marcingy commentedNew version of patch that no longer makes the theme active by default and instead prompts the user to visit the admin page to enable it. This then treats themes the same as modules. The validation on the admin page then ensures that we can't enable an unsupported theme.
Comment #13
marcingy CreditAttribution: marcingy commentedadding tag
Comment #14
marcingy CreditAttribution: marcingy commentedMoving to d8 as we are not going to get this in due to string changes
Comment #15
quicksketchPer #1050616: Figure out backport workflow from Drupal 8 to Drupal 7, major and critical bugs are now preventing development in other areas of Drupal core. Unless this issue is actually a bug (data loss, functionality not working), we can mark this as a "major feature request" or a "normal bug", but not a "major bug".
Comment #16
adammaloneJust taking a look at this as it happened to me today. I've rewritten some of the other patches in this queue and made a patch for D8.
First core patch so I hope this works!
Comment #17
adammaloneJust altered the order a little bit to make it more consistent with existing code.
Comment #19
adammalone#17: disable-themes-when-no-base-or-engine-304540-17.patch queued for re-testing.
Comment #20
jaredsmith CreditAttribution: jaredsmith commentedRe-rolled this patch to make it work correctly under Drupal 8 dev, and tested the functionality with both a sub-theme with a missing base theme, as well as with a theme that was missing its engine.
Comment #21
adammaloneNot quite sure a reroll was required to rename elements in an array...
Comment #22
BrockBoland CreditAttribution: BrockBoland commented@typhonius: Yes, the reroll was necessary, since the array key for base theme name is
base theme
, notbase
.This patch works. I'm working on adding a test case for it now.
Comment #23
BrockBoland CreditAttribution: BrockBoland commentedTwo files are attached here:
304540-23_test_only.patch
contains only the test case, so it will fail tests.304540-23_fixed.patch
contains the test case and the fix from #20, so it will pass tests.Comment #25
BrockBoland CreditAttribution: BrockBoland commentedI uploaded patches in the wrong order, so the test-only patch triggered a status change when it (correctly) failed.
Comment #26
BrockBoland CreditAttribution: BrockBoland commentedLast one, I promise: the last patch mis-used
t()
in the test case.Comment #28
BrockBoland CreditAttribution: BrockBoland commented#26: 304540-26.patch queued for re-testing.
Comment #29
sarahjean CreditAttribution: sarahjean commentedI tested this patch, it applied and now instead of 'Enable' 'Enable and Set default' I see 'This theme requires the theme engine ets to operate correctly.'
Comment #30
catchLooks good to me. Committed/pushed to 8.x.
Comment #31
xjmFollowup here: #1726810: Theme page test module not marked as "hidden"
Also, was this supposed to be backported? I see string additions, but no string changes, and it is a bugfix. (#14 moved it to D8 without tagging for backport on account of strings.)
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedIn principle it seems backportable to me.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedComment #34
adammaloneThere's an issue with this when the you have a subtheme of a subtheme.
Let us use 3 themes: A, B & C
C is a subtheme of B, which is a subtheme of A.
Assume themes B and C are present on the site yet A is not. The code in system.admin.inc only checks the parent base theme, not any further up the chain.
A real world example of this would be if someone created a rubik subtheme and then downloaded rubik but not tao. The theme page would allow the user to enable the rubik subtheme (but not enable rubik).
Although my patch doesn't cover tests, it does have some logic in it that detects whether all base themes for a theme are present.
Comment #35
adammaloneAs an explanation:
I noticed that $theme->base_themes is an array of base themes which, if the base theme is not present the value is NULL for the key of that base theme. By using array_filter with no arguments it removes any empty elements hence making the array different to that of a non filtered array of base themes.
Comment #36
adammaloneAdded a patch that includes a test for the above. I've not written a test for core before so I've piggy-backed on the test that was committed to HEAD previously for detecting invalid base theme with my basic test knowledge.
Comment #37
podarok#36
-}
No newline at end of file
trailing whitespace
Comment #38
adammaloneRemoved the whitespace and had a go at adding a newline. Not sure if that worked though...
Comment #39
adammaloneComment #41
adammalone#38: 304540-38.patch queued for re-testing.
Comment #42
adammaloneReroll and retest.
Comment #43
adammaloneSince I'm adding in another test I've been advised by larowlan to test a patch that includes just the new test and a patch that includes the new test and the fix.
This is to ensure that the test is actually testing what is being fixed.
Comment #46
jhedstromComment #47
ravi.khetri CreditAttribution: ravi.khetri commentedRe-rolled.
Comment #48
ravi.khetri CreditAttribution: ravi.khetri commentedComment #49
ravi.khetri CreditAttribution: ravi.khetri commentedRerolled
Comment #51
manningpete CreditAttribution: manningpete commentedPatch applies.
Comment #52
jhedstromThe reroll above only includes the test, not the fix.
Comment #53
jyotisankar CreditAttribution: jyotisankar commentedRerolled
Comment #55
jyotisankar CreditAttribution: jyotisankar commentedComment #57
DuttonMa CreditAttribution: DuttonMa commentedComment #58
DuttonMa CreditAttribution: DuttonMa commentedComment #59
sudhanshug CreditAttribution: sudhanshug commentedrerolled patch
Comment #60
sudhanshug CreditAttribution: sudhanshug commentedComment #63
nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commentedRerolled
Comment #65
r.nabiullin CreditAttribution: r.nabiullin at Skilld commentedRe-rolled patch from comment #42
Comment #67
r.nabiullin CreditAttribution: r.nabiullin at Skilld commentedcode totally outdated so manual reroll
Comment #68
r.nabiullin CreditAttribution: r.nabiullin at Skilld commentedComment #69
andypostComment #70
andypostplease add the line break
Comment #71
r.nabiullin CreditAttribution: r.nabiullin at Skilld commentedadded the line break
Comment #72
andypostLooks ready
Comment #75
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x, thanks!
Moving to 7.x for backport.
Comment #76
therealssj CreditAttribution: therealssj commentedPatch rerolled for 7.x
Comment #78
therealssj CreditAttribution: therealssj commentedMissed a few files there.
Here is the full reroll for 7.x
Comment #79
therealssj CreditAttribution: therealssj commentedTest fix.
Tested Reroll for 7.x