Problem/Motivation

Now that hooks can be oop and in their own file there is no need for theme-settings.php

Let's convert all but the one test and deprecate this file.

Steps to reproduce

N/A

Proposed resolution

There are only two remaining.

Olivero
A test theme
Let's leave the test theme for legacy tests
Let's convert olivero to it's own oop hook class
Let's deprecate the file if it is found in theme hook collector pass.

For the legacy ignore deprecations also add a comment that we need to just convert the file when we drop support.

Remaining tasks

Review

User interface changes

N/A

Introduced terminology

N/A

API changes

theme-settings.php are no longer supported.

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#7 3580152-deprecate-theme-settings-php-5.patch6.58 KBrajivgandhi chinnakrishnan

Issue fork drupal-3580152

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:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
Parent issue: » #3554670: [meta] Eliminate core .theme files
nicxvan’s picture

nicxvan commented
Title: Deprecate theme-settings.php » Deprecate theme-settings.php files
debdeep.mukhopadhyay’s picture

debdeep.mukhopadhyay commented

I am working on this.

debdeep.mukhopadhyay opened merge request !15143

debdeep.mukhopadhyay’s picture

debdeep.mukhopadhyay commented

Hi @nicxvan,

I worked on the issue related to deprecating theme-settings.php in the Olivero theme.

While setting up the issue fork on Drupal 12.x-dev, I faced some dependency and environment related conflicts which made it difficult to properly test the theme settings behaviour. To avoid blocking progress, I recreated the setup on a stable Drupal 11 environment and verified the approach there.

I moved the logic from theme-settings.php to a new OOP hook class (OliveroThemeSettingsHooks) using the Hook attribute, registered it via olivero.services.yml, removed the legacy file, rebuilt cache, and confirmed that the Olivero appearance settings page loads and saves correctly without errors.

Now I am continuing the work on the issue fork branch to prepare a Drupal 12 compatible patch/MR.

Please let me know if anything else needs to be validated before I proceed further.

Thanks.

rajivgandhi chinnakrishnan’s picture

rajivgandhi chinnakrishnan commented
StatusFileSize
new 3580152-deprecate-theme-settings-php-5.patch6.58 KB

Converts olivero/theme-settings.php to an OOP hook class (ThemeSettingsHooks) using the #[Hook] attribute, then deletes the procedural file. A deprecation notice (E_USER_DEPRECATED) is added to HookCollectorPass so any contrib or custom theme still shipping a theme-settings.php gets a clear, actionable warning on container rebuild. The theme_test theme's file is intentionally left untouched to preserve legacy test coverage.

nicxvan’s picture

nicxvan commented

@debdeep.mukhopadhyay thank you, I'm not sure what you mean about 11.x, the MR you posted is against main which is what will become drupal 12 so I think it is correct.

That conversion is correct, we do need to do the actual deprecation.
We do not need the services file for the theme, in fact it does nothing so we should delete it.
For the deprecation it should happen in ThemeHookCollectorPass around line 224, if $isThemeSettings is TRUE I would output a deprecation message.

This issue will need a CR explaining the changes as well.

Edited to add: We also need to resolve the PHPCS errors:

FILE: /builds/core/themes/olivero/src/Hook/OliveroThemeSettingsHooks.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------
  8 | ERROR   | [x] Missing class doc comment
    |         |     (Drupal.Commenting.ClassComment.Missing)
 17 | WARNING | [ ] t() calls should be avoided in classes, use
    |         |     \Drupal\Core\StringTranslation\StringTranslationTrait and
    |         |     $this->t() instead
    |         |     (DrupalPractice.Objects.GlobalFunction.GlobalFunction)

@rajivgandhi chinnakrishnan we don't use patches any longer for core contributions. I am going to hide it.
Also that patch has several issues with it so please don't push it to the MR without discussion.
Briefly, it's the wrong collectorPass, it mentions a drupal version no longer supported, I have no idea what version of core it's against since the olivero theme-settings don't seem to align with 11 or main.

debdeep.mukhopadhyay’s picture

debdeep.mukhopadhyay commented

Hi @nicxvan, @rajivgandhi chinnakrishnan,

Thank you for the guidance and review comments.

After the last feedback, I re-verified the implementation by testing the OOP conversion of theme-settings.php in both Drupal 11 (for behaviour comparison) and current main (future Drupal 12). I also updated the approach based on suggestions — removing the unnecessary services.yml, addressing PHPCS feedback, and ensuring the deprecation logic is placed in the correct compiler pass (ThemeHookCollectorPass).

The changes are now aligned with the current core structure and expected deprecation flow.
Kindly let me know if anything else needs improvement — happy to update further.

Thanks for reviewing 🙏

nicxvan’s picture

nicxvan commented

We will need a test for the deprecation too, you can add a theme settings file to the test procedural hook theme

nicxvan’s picture

nicxvan commented
Title: Deprecate theme-settings.php files » [pp-1] Deprecate theme-settings.php files
Issue summary: View changes
Status: Active » Postponed
Related issues: +#3581218: Deprecate .theme file extension

I pushed up a fix for the deprecation message format.

I only see one other sprintf in deprecations, but I left it here for now.

We do still need to add a test for the deprecation path since it's unique, but we should do this after the actual .theme deprecation.

#3581218: Deprecate .theme file extension

nicxvan’s picture

nicxvan commented

Why did you change the hook implementation so much?

nicxvan’s picture

nicxvan commented

I'm very confused, both the MR and the patch have incorrect versions of the conversion. Can you both please provide information on how you did the conversion and why over 100 lines were deleted?

debdeep.mukhopadhyay’s picture

debdeep.mukhopadhyay commented

Hi @nicxvan,
can you please little eleborate which part conversion seems incorrect,it's help me better for understand and for the right correction.

nicxvan’s picture

nicxvan commented
Issue tags: +11.5.0 release priority
nicxvan’s picture

nicxvan commented
Issue tags: +Needs tests

I rebased and updated the deprecation message, we still need tests.

nicxvan’s picture

nicxvan commented
Title: [pp-1] Deprecate theme-settings.php files » Deprecate theme-settings.php files
Status: Postponed » Needs work

nicxvan changed the visibility of the branch 3580152-deprecate-theme-settings.php-files to hidden.

nicxvan opened merge request !16664

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
Issue tags: -Needs tests

I closed the other MR cause I think something was broken there, I had already addressed the deprecation issue that @oily pointed out, but it was the right thing. The comment about the settings language should be a follow up.

This is green now and ready for review!

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Discussed this with @nicxvan in slack.

Had questions about if D12 was too soon but he made a good point that it's not a lot of code and not complex enough to push out.
2nd question was the use of " move to the .theme file" since those are going away and he reminded me that .theme won't go away till D13.

Rest looks good.

mstrelan’s picture

mstrelan commented

It seems strange that we would recommend moving code to a .theme file for D12 when .theme file extensions have been deprecated.

nicxvan’s picture

nicxvan commented

The primary recommendation is to convert the hooks.

The recommendation to move to the .theme file is only if the theme has to support lower than 11.3 simultaneously with 12.

nicxvan’s picture

nicxvan commented

I updated the CR for clarity.

mstrelan’s picture

mstrelan commented

Thanks for updating the CR. For me, the CR was already clear, it's the deprecation message that bothered me. I think many would go for the quick fix of moving it, only to have to move it again later. That may be fine though, and is supported through D12.

nicxvan’s picture

nicxvan commented

Yeah it's just one hook and it's not super common to use this feature.

I think it's fine if some percentage of people copies and pastes the function then later has to convert it.

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Yes I think this is reasonable to deprecate for removal in 12.0.0, it's very rarely used, and helps us simplify the bc layer that we do want to support through to Drupal 13.

Committed/pushed to main, thanks!

Needs a backport MR for 11.x

  • catch committed 9758a3ba on main 
    task: #3580152 Deprecate theme-settings.php files

By: nicxvan
By:...

nicxvan opened merge request !16876

nicxvan’s picture

nicxvan commented
Status: Patch (to be ported) » Reviewed & tested by the community

Backport should be ready!

Conflict was just in the themesettings test.

The failure looks like a random functionalJS test: Drupal\FunctionalJavascriptTests\BrowserWithJavascript

  • catch committed 0757aebb on 11.x 
    task: #3580152 Deprecate theme-settings.php files

By: nicxvan
By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.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.

nicxvan’s picture

nicxvan commented
Issue tags: +11.5.0 release highlights

Status: Fixed » Closed (fixed)

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