Problem/Motivation

Themes can now be OOP.
Let's convert all remaining themes except claro, olivero and starterkit.

Steps to reproduce

Open *.theme

Proposed resolution

Use rector to convert

Remaining tasks

Review

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-3575585

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 opened merge request !14904

nicxvan changed the visibility of the branch 3575583-convert-olivero-to to hidden.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented

Would it be safer to break umami and stable9 into their own issues?

nicxvan’s picture

nicxvan commented

I don't even think we need to split claro and olivero out, we converted all core modules in one go. I split it out to try to get more reviews.

I would keep the remaining ones together to minimize disruption though.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

They appear like good conversions though.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Added a question about stable9. Umami is explicitly only a demo so BC there isn't necessary.

nicxvan’s picture

nicxvan commented

I added BC for stable9, it's good to have an example for that too since it's different than BC for modules.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed cc89478fae0 to main and 7ba1a2447d4 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.

  • longwave committed 7ba1a244 on 11.x 
    refactor: #3575585 Convert remaining themes to OOP

By: nicxvan
By:...

  • longwave committed cc89478f on main 
    refactor: #3575585 Convert remaining themes to OOP

By: nicxvan
By:...

longwave closed merge request !14904

Status: Fixed » Closed (fixed)

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

donquixote’s picture

donquixote commented

Hi,
just my 5 cents in hindsight.
I think I super disagree about removing the test functions for procedural preprocess.

I see we did re-add procedural hooks for testing in #3579629: Add procedural hooks to test theme to test collection and invoking legacy hooks. But this does not have the coverage of the old hooks, with sub-themes.
In fact we should have added more procedural examples, not less, to cover scenarios that were previously not covered.

nicxvan’s picture

nicxvan commented

Feel free to open an issue for that in the theme queue and ping me with it if you want to work on adding additional coverage.

donquixote’s picture

donquixote commented

Feel free to open an issue for that in the theme queue and ping me with it if you want to work on adding additional coverage.

I am currently investigating if the order of procedural implementations has changed in some scenarios.
I will open an issue based on my findings.

But, the best time to add these additional tests would have been before the introduction of OOP preprocess.
Now if we find out that the order has changed from 11.2 to 11.3, it will be hard to fix, because then we'd break Drupal 11.3 behavior.

But, I should do my homework first, before I start a drama :)