Use hook_form_system_theme_settings_alter() for theme settings

[docwilmot]

Description of the bug

Backdrop themes which provide a theme-settings.php have form elements in that file outside of a function definition. This seems unique to Backdrop, and is not a pattern seen elsewhere in Backdrop code.

Steps To Reproduce

See any of the files at https://api.backdropcms.org/api/backdrop/1/search/theme-settings.php

Expected behavior

The form elements should be in hook_form_system_theme_settings_alter(). Although the current state works, it is quite nonstandard and difficult to figure out why it is this way.

[indigoxela]

I'm not 100% sure if I would call it a bug, but I fully second the intention of this Issue.

It looks freaky to add / include form items like that. Let's cleanup this legacy code.

@docwilmot do you also suggest to deprecate this way long-term?

[docwilmot]

I'm not 100% sure if I would call it a bug

Agreed, but it didnt seem like a Feature request, Question or Security issue. 😄

@docwilmot do you also suggest to deprecate this way long-term?

I think this freakiness is just a curious side effect of how the include code is implemented. I think if we dont use it and dont mention it we can just ignore it. Wont hurt that its possible.

[docwilmot]

I've wrapped the theme-settings.php code from Bartik and Basis in hook_form_system_theme_settings_alter(). But turns out that there was a deprecated function called bartik_form_system_theme_settings_alter() already, that was being called by color_form_system_theme_settings_alter(). I've had to rename it, which should be safe since it was deprecated so no one should have been using it.

Would like some more input though.

[klonos]

Agreed, but it didnt seem like a Feature request, Question or Security issue. 😄

That's why why have "tasks" for 😉 ...tasks can also get in in bug fix releases.

do you also suggest to deprecate this way long-term?

I think this freakiness is just a curious side effect of how the include code is implemented. I think if we dont use it and dont mention it we can just ignore it. Wont hurt that its possible.

I agree 👍

[klonos]

I agree with this, but the PR is currently breaking many tests.

[indigoxela]

Yep, the failing tests are related, so back to "needs work".

And we also have to take care that this change doesn't break subthemes.

[docwilmot]

And we also have to take care that this change doesn't break subthemes.

The solution to fix the test errors suggests that administration pages for subthemes of Bartik may be affected. The presence of a $form['color'] block in their theme-settings files may override the color settings at admin/appearance/settings/THEME. In Basis it was wrapped in a if (module_exists('color')) { so it wouldnt be a problem.

The site wouldnt be broken, as the theme itself would be fine, just the admin page color settings could be unavailable unless that key is renamed or removed.

Note though that I say "may" be broken, because this only seemed to happen on Simpletest, and then still only if the testing profile is used. On my local, this PR does not affect the settings pages, and in testing with the Standard profile everything also worked fine. I cannot imagine why there would be websites based on the testing profile out there, but cant be sure?

[docwilmot]

Needs review again

[ghost]

If we're going to move theme settings code into a proper hook, why not also move it into the template.php file along with the rest of the hook.implementations...?

[indigoxela]

If we're going to move theme settings code into a proper hook, why not also move it into the template.php

Currently this issue is marked as bugfix. Bigger changes to the whole theme settings logic might make it a 2.x issue. Not sure, if that's what we want to achieve here.

@docwilmot many thanks for updating the PR! There's still a conflict, that must be resolved, though. Your branch is 488 commits behind, which is quite a lot.