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

Command icon 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:

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan’s picture

Issue summary: View changes
berdir’s picture

Status: Active » Needs work

Looks like something isn't working yet.

nicxvan’s picture

Yeah it needs work I'll add something to the original cr about this hook.

nicxvan’s picture

This 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.

nicxvan’s picture

I 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.

nicxvan’s picture

Issue summary: View changes
Status: Needs work » Needs review

nicxvan’s picture

Issue summary: View changes

Created test only branch since this needs the conversion for the failures to show.

godotislate’s picture

A couple comments, not RTBC blockers. OK to RTBC if suggestions declined.

godotislate’s picture

Thanks for indulging the nits. There's a test failure, looks random. RTBC as long as that passes on re-run.

godotislate’s picture

Status: Needs review » Reviewed & tested by the community

Tests are passing, looks good now.

  • catch committed 325f82b3 on 11.3.x
    fix: #3560833 Create better way to manage theme setting alters
    
    By:...

  • catch committed d73f225c on 11.x
    fix: #3560833 Create better way to manage theme setting alters
    
    By:...
catch’s picture

Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

This 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!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

cilefen’s picture

It looks like this may have introduced a regression, #3581226: 'Class "olivero_form_system_theme_settings_alter" does not exist' error.