Problem/Motivation
ThemeSettingsForm manually calls hook_form_system_theme_settings_alter for the theme, we need to invoke this so the OOP version is picked up, or move it to it's own function or process.
// Call theme-specific settings.
$function = $theme . '_form_system_theme_settings_alter';
if (function_exists($function)) {
$function($form, $form_state);
}
Steps to reproduce
Convert test_theme_theme_form_system_theme_settings_alter
See core/modules/system/tests/src/FunctionalJavascript/ThemeSettingsFormTest.php fail
Proposed resolution
Change ThemeSettingsForm to call alter instead of checking for function existence directly.
Also add theme-settings.php to collection for procedural hooks.
Remaining tasks
Review
Review test only: https://git.drupalcode.org/project/drupal/-/merge_requests/14061/diffs
User interface changes
N/A
Introduced terminology
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Issue fork drupal-3560833
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3560833-test-only
changes, plain diff MR !14061
- 3560833-create-better-way
changes, plain diff MR !14014
Comments
Comment #2
nicxvan commentedComment #4
nicxvan commentedComment #5
berdirLooks like something isn't working yet.
Comment #6
nicxvan commentedYeah it needs work I'll add something to the original cr about this hook.
Comment #7
nicxvan commentedThis is a weird one! We have to include theme-settings.php for scanning.
This is now working but tests are still failing, I'm not sure why.
Edit to clarify: with these changes manually visiting the test theme theme settings page the settings are there as expected. The tests are still failing though.
Comment #8
nicxvan commentedI spent some more time debugging and the html is definitely there, I printed the page in the test:
<input data-drupal-selector="edit-custom-logo-fids" type="hidden" name="custom_logo[fids]" value="1">I do see
Oops, something went wrong. Check your browser's developer console for more details.in that page too, so something is going wrong I think but it doen't replicate in a manual test.Comment #9
nicxvan commentedComment #11
nicxvan commentedCreated test only branch since this needs the conversion for the failures to show.
Comment #12
godotislateA couple comments, not RTBC blockers. OK to RTBC if suggestions declined.
Comment #13
godotislateThanks for indulging the nits. There's a test failure, looks random. RTBC as long as that passes on re-run.
Comment #14
godotislateTests are passing, looks good now.
Comment #17
catchThis looks fine for during RC, and if we didn't commit it, we'd need to explain why it doesn't work, and then even if we committed it to a patch release of 11.3.x, we'd need to explain that it works from a certain patch release. So... let's commit it now.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Comment #21
cilefen commentedIt looks like this may have introduced a regression, #3581226: 'Class "olivero_form_system_theme_settings_alter" does not exist' error.