Updated: Comment #47
Problem/Motivation
First the setup to recreate this problem:
- A sub theme set to the Admin theme
- Another sub theme (with same base theme as admin theme) set as the main front end theme
- Both sub themes have custom theme settings
When changing the theme settings for the front end theme, they are saved and take effect, however when the settings page reloads all the settings for the admin theme are loaded instead, not the front end themes settings.
To show this clearly please watch this video.
--
@bfroehle summarizes the problem technically in #2:
The theme settings pages are built by system_theme_settings(). This function generates the form by running THEME_system_theme_settings_alter() for the theme you are trying to edit, as well as all of that theme's parents. In each case, $GLOBALS['theme_key'] is set to the name of the theme we're trying to edit.After the form is initially built, we reset $GLOBALS['theme_key'] to the name of the administration theme, and finish processing the form by calling drupal_alter() which then calls THEME_system_theme_settings_alter() and overwrites the correct sub-theme values.
Proposed resolution
We need to move away from the drupal_alter() namespace in this case as form alter hooks are the main cause of this issue.
Remaining tasks
- There is a patch attached to comment #43 that needs help to get passing tests
User interface changes
N/A
API changes
@JohnAlbin provides a good explantion of the API changes in #34
As discussed in https://drupal.org/node/1164790 if we rename system_theme_settings() to system_theme_config() and THEME_form_system_theme_settings_alter() to THEME_theme_config() (That would also make the module's hook alter be hook_form_system_theme_config_alter().)
Related Issues
N/A
Comment | File | Size | Author |
---|---|---|---|
#62 | core-943212-62.patch | 12.93 KB | savkaviktor16@gmail.com |
#43 | core-943212-7446826.patch | 11.81 KB | arknoll |
#36 | 943212-36-admin-themes.patch | 14.53 KB | JohnAlbin |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedI haven't tried reproducing, but if the bug is as described, and does not happen in D6, I think it qualifies as critical.
Comment #2
bfroehle CreditAttribution: bfroehle commentedThe theme settings pages are built by
system_theme_settings()
. This function generates the form by runningTHEME_system_theme_settings_alter()
for the theme you are trying to edit, as well as all of that theme's parents. In each case,$GLOBALS['theme_key']
is set to the name of the theme we're trying to edit.After the form is initially built, we reset
$GLOBALS['theme_key']
to the name of the administration theme, and finish processing the form by callingdrupal_alter()
which then callsTHEME_system_theme_settings_alter()
and overwrites the correct sub-theme values.Comment #3
bfroehle CreditAttribution: bfroehle commentedWill certainly fix the problem, but I think it's not at all the correct solution.
Comment #4
webchickAt this point, critical == if we don't fix it before RC, people will die. I don't see any reason this couldn't be fixed after RC1, or even in 7.2.
Comment #5
bfroehle CreditAttribution: bfroehle commentedI'm retitling to give a better description of the problem. I disagree a bit with @webchick here -- the cleanest solution might be to rename THEME_form_system_theme_settings_alter. Right now that function is serving two distinctly different uses:
drupal_alter
but rather directly fromsystem_theme_settings
.system_theme_settings
, via the standarddrupal_alter
/ form generation framework.Any solution to this bug, short of renaming THEME_form_system_theme_settings_alter, is likely going to involve a hack-ish "don't call theme hooks when altering form_system_theme_settings."
Feel free to bump it back down to major if you disagree.
Comment #6
marcingy CreditAttribution: marcingy commentedSee http://drupal.org/node/974066 - webchick has already given her opinion on this issue.
Comment #7
bfroehle CreditAttribution: bfroehle commentedHere's a patch which changes drupal_alter to not call THEME_form_system_settings_form_alter. It fixes the problem locally for me.
Comment #8
Jeff Burnz CreditAttribution: Jeff Burnz commentedPatch in #7 is working for me on latest DEV.
Comment #9
sunPatch in #7 won't fly.
A test would help to resolve this issue + I'd like to see a test-only patch first that confirms the bug.
Comment #10
Jeff Burnz CreditAttribution: Jeff Burnz commentedsun suggested we write a test for this first, tagging.
Comment #11
Jeff Burnz CreditAttribution: Jeff Burnz commentedcross post
Comment #12
bfroehle CreditAttribution: bfroehle commentedEdit: Oops, this will never pass since I messed up the
assertFieldChecked
call.Comment #13
bfroehle CreditAttribution: bfroehle commentedThis set of tests should actually work properly. See the note in #12 to know what was wrong with the previous attempt.
Comment #15
bfroehle CreditAttribution: bfroehle commentedTest will need to be re-rolled once #953336: Contributed modules are not able to test theme-related functionality goes in. (Also, this means we could put the test themes in
modules/system/....
instead of inthemes/tests
.Comment #16
bfroehle CreditAttribution: bfroehle commented#13: 943212-tests-only-but-tests-work.patch queued for re-testing.
Well, here is your test-only patch.
Comment #18
dvessel CreditAttribution: dvessel commentedJeff Burnz pointed to this issue from my duplicate post. #1135794: Introduce hook_settings() for themes
Doing anything to drupal_alter to fix this specific issue should not be done. It was a mistake to rename hook_settings so it shares the same namespace as drupal_alter.
Comment #19
sunSorry if I'm missing something, but I'm 99% sure that this issue can be 100% resolved by simply removing these lines from system_theme_settings():
100% sure including all edge-cases pertaining to themes.
Comment #20
dvessel CreditAttribution: dvessel commentedSun, note the $theme_key switching. That key is used to set focus on the theme specific form. The theme used to render the page and the theme settings form for given theme are not always the same.
Comment #21
yoroy CreditAttribution: yoroy commentedWell, confirmed so subscribe. Running into this building my Seven sub theme (http://drupal.org/sandbox/yoroy/1131280), tweaking the styling of the toolbar. In admin I see my toolbar changes, in the front end I see default Seven styling.
It seems the 3 previous comments above need some more discussion still.
Comment #22
catchouch, subscribing.
Comment #23
bfroehle CreditAttribution: bfroehle commentedFor D8, I think we should rename THEME_form_system_theme_settings_alter() to something not in the drupal_alter() namespace.
For D7, I think we're hosed. One very hackish option would be to require theme developers to change the design pattern to be:
to rely on the fact that the drupal_alter() call has $form_id set, but the direct call from system_theme_settings() does not.
Comment #24
dvessel CreditAttribution: dvessel commentedHere’s another workaround. Not sure the comments make sense.
Comment #25
Jeff Burnz CreditAttribution: Jeff Burnz commented@dvessel - that's working for me, absolutely awesome, for the first time I can actually use my own admin theme. Thank-you very much.
Comment #26
JohnAlbinNice work-around. Sorry about hosing all the admin themes in D7. :-p
Comment #27
aquariumtap CreditAttribution: aquariumtap commentedsubscribing. soon to try dvessel's workaround... thank you, sir
Comment #28
bfroehle CreditAttribution: bfroehle commentedWe need to agree on a fix for 8.x. Shall we rename THEME_form_system_theme_settings_alter() to something not in the drupal_alter namespace, say THEME_theme_settings() ?
There are some brief suggestions for a fix #1135794: Introduce hook_settings() for themes.
Comment #29
bfroehle CreditAttribution: bfroehle commentedComment #30
RobLoachRemove theme_form_FORMID_alter() and instuct to use the module hook instead? Or, like suggested above, rename theme_form_FORM_ID_alter() to theme_theme_form_FORMID_alter() or something.
Comment #31
ryan.gibson CreditAttribution: ryan.gibson commentedAlright so here is a reroll from #11. I warn you, this will likely be butchered.
Comment #32
ryan.gibson CreditAttribution: ryan.gibson commentedAnd here's a reroll of the patch from #7.
Comment #34
JohnAlbinOver in #1164790: “Settings” is not an immediately-expected term there is a discussion that the "settings" link is not clear. If we rename it to "configure" we can change system_theme_settings() to system_theme_config() and THEME_form_system_theme_settings_alter() to THEME_theme_config(). (That would also make the module's hook alter be hook_form_system_theme_config_alter().)
2 problem birds. 1 patchy stone.
Comment #36
JohnAlbinWow. Didn't take long for the patch to stop applying. Retrying.
Comment #37
treksler CreditAttribution: treksler commentedI was just bitten by this bug in Drupal 7. The key annoyance with this bug is that the form alter hook runs twice. Once from system_theme_settings() and again from drupal alter(). In my case, this behaviour caused it to attach form validate and submit alters twice, which then tried to save and delete custom CSS files twice. Super annoying.
Renaming the theme settings function to not conflict with the hook form_alter() namespace, as per patch #36, is a no brainer.
Drupal 7 hook_form_alter() documentation should also warn about using hook_form_system_theme_settings_alter() from your theme, and suggest the workaround in #24 to bail out if $form_id is not set.
Comment #38
star-szr#36: 943212-36-admin-themes.patch queued for re-testing.
Comment #40
star-szrTagging.
Comment #41
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #42
arknoll CreditAttribution: arknoll commentedportland sprint, working on reroll
Comment #43
arknoll CreditAttribution: arknoll commentedRerolled #36.
Comment #45
star-szr#43: core-943212-7446826.patch queued for re-testing.
Comment #47
nathangervais CreditAttribution: nathangervais commentedIssue summary updated.
Comment #47.0
nathangervais CreditAttribution: nathangervais commentedUpdated issue summary.
Comment #48
star-szrIssue summary looks very good to me, nice work @nathangervais!
Comment #49
YesCT CreditAttribution: YesCT commented#36 had 7 files changed, 39 insertions, 37 deletions.
#43 has 6 files changed, 32 insertions, 29 deletions.
I'm not sure
but removing the needs reroll tag.
Comment #49.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #50
steinmb CreditAttribution: steinmb commentedComment #51
cilefen CreditAttribution: cilefen commentedI realized this may be the situation in Drupal 8 now. Bartik is a sub-theme of Classy, and so is Seven. I tried to reproduce the issue in the summary in HEAD but could not. Can someone more knowledgable of the theme system check if this is still an issue?
Comment #52
lauriiiAfter seeing the code behind this, I wouldn't be amazed if this would still happen. Anyway this API is very fragile so we should at least investigate it has test coverage.
Comment #54
xjmI'd agree with #52 -- probably move this issue back to D7 only if it is not reproducible in D8, and create a normal followup in D8 for expanding the test coverage (using test themes, not Classy and its children).
Comment #55
JohnAlbinThe only way to fix this fragile code is to change the API. The theme settings need to be added to the form _before_ the form finishes building so that modules and themes can _alter() that form.
That means the theme that owns the settings should be using a custom hook name (and not a pseudo- _alter() hook name). That kind of change is a backwards-incompatible change.
Which means we would need to write it in such a way that the old API is deprecated without being removed.
While the bug described may not happen any more due to Forms API changes, it is DEFINITELY still true, that a theme's THEME_form_system_theme_settings_alter() gets run _twice_. That could cause all sorts of unpleasantness if you aren't careful. I'm still using dvessel's work-around (#24) in my Drupal 8 themes.
Comment #58
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedI'm updating my theme to D8 and I can confirm this problem still exists. I make extensive use of theme settings and inside my hook_form_system_theme_settings_alter implementation I run a plugin system that includes files and those files contain functions within the hook_form_system_theme_settings_alter namespace.
Due to double loading that still exists in D8 this produces fatal errors due to redeclaring of the same functions. If I change my plugin system from this:
To use require_once like this:
..the custom theme settings and functions in the included files are not loaded at all.
I use a similar workaround to #24, which I now sadly have to copy into my Drupal 8 theme-settings.php file:
Comment #60
andypostComment #62
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedRe-rolled (redone according to current structure) #43's patch. I'm not sure that's done properly, but still.
Comment #63
markhalliwellI'm not entirely sure why the above patch is changing all the paths. IMO, this seems like overkill and a massive BC break, if only for all the [external] documentation purposes.
Also, renaming a hook, whether it clashes or not doesn't seem like the most appropriate thing to do either.
This has been a well known, and well documented, issue in contrib projects for years now.
Changing this now seems like it may cause more damage than its worth.
I'd rather we implement something like #2869859: [PP-1] Refactor theme hooks/registry into plugin managers, where themes would have the ability to create their own settings inside an OO class, thus bypassing the whole "hook clashing" issue altogether.
I'm tempted to just mark this issue as won't fix but will set it back to CNW in case someone else can come up with a better, more non-disruptive fix.
Comment #64
markhalliwellComment #75
tyler36 CreditAttribution: tyler36 at Inter Quest commentedThe video in the description requires flash to play.
Marking it as "wont fix" based on:
- was original open in 2010
- has had no activity for 5 year
- @markhalliwell comment in #63
Should anyone decided to reopen, please note the current (5 year old) patch has multiple references to "bartik", a theme that is no longer shipped with Drupal.