Problem/Motivation

Postponed on: #3581218: Deprecate .theme file extension
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

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.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

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.