theme_get_setting() in theme.inc contains some comments which seem to advocate hacking core files.

I suggest removing these two lines:

    // To add new global settings, add their default values below, and then
    // add form elements to system_theme_settings() in system.admin.inc.

There are modules in contrib (touch_icons, noggin) which extend themesettings variables, and they do it through hook_form_alter().

CommentFileSizeAuthor
#1 theme_get_setting-comments-1762572-1.patch603 bytesandrewmacpherson

Comments

andrewmacpherson’s picture

andrewmacpherson commented
StatusFileSize
new theme_get_setting-comments-1762572-1.patch603 bytes

patch for D8

andrewmacpherson’s picture

andrewmacpherson commented
Status: Active » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Component: documentation » theme system

Hm. I think this is a comment for core developers, not an inducement for hacking core. Moving to different component for comment, as this is not purely a docs issue.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Version: 8.0.x-dev » 7.x-dev
Issue summary: View changes

I believe the theme settings system has changed thoroughly enough for this to no longer apply, bumping to D7 at least.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Closed (works as designed)

I do not actually agree that these comments are advocating hacking core. I think they are reminding people who need to make a Core patch to add a new setting, how they would go about doing this. I do not think we should remove this comment.

We could reword it but... at this point in 7.x probably not worth it.

Feel free to reopen and propose a rewording patch if you disagree, but I don't think I'd commit the removal patch.

star-szr’s picture

star-szr
he/him
Primary language English
commented

Yup, that makes sense to me too. Thanks @jhodgdon.