Problem/Motivation

Themes can now be OOP.
Let's convert Claro.

Steps to reproduce

Open claro.theme

Proposed resolution

Use rector to convert

Remaining tasks

Review

Follow ups:
Remove check for core media: formMediaFormAlter
Create follow up for organization and DI
Create follow up to replace remaining functions:

  • _claro_convert_link_to_action_link
  • _claro_preprocess_file_and_image_widget
  • claro_system_module_invoked_library_info_alter
  • claro_system_module_invoked_theme_registry_alter

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

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
Parent issue: » #3554670: [meta] Eliminate core .theme files

nicxvan opened merge request !14901

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

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

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Added a few specific points.

I think here too, DI is doable and this is much bigger than olivero, but I still think we should do some basic amount of splitting here. Finding a good grouping for all the preprocess stuff is not trivial, but I'd really like the form alters and other actual hooks to be separate from all the preprocess stuff? ClaroPreprocessHooks, ClaroFormHooks, ClaroHooks?

nicxvan’s picture

nicxvan commented

Splitting codes out after an automated conversion really feels out of scope, same with DI.

The thing is with automated conversions you can't rebase, you just have to repeat the process.

The more manual changes after conversion the more risk you miss or break something.

All of the automated conversions have followed this pattern:

  • Run rector
  • Fix code sniff rules including add StringTranslationTrait
  • Fix phpstan issues
  • Fix any failing tests
  • Create followups for anything else

This has been very successful, I'd hesitate to change the process since it's not very sustainable.
I know introducing DI has taken far longer than we'd like in module hooks so it's tempting to ensure it's done first here but I think it won't linger as much here.

I fixed the references and I can try reverting the readme if you think it's important.

nicxvan’s picture

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

Addressed the feedback except DI and organization.

I explained why in the previous comment and @berdir mentioned in slack he wouldn't push for it further.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Needs regenerating

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

It was actually a change in a comment I manually updated so I was able to rebase normally.

It was this change: https://git.drupalcode.org/project/drupal/-/merge_requests/14876/diffs#c...

That isn't correct so I changed it to Implements hook_preprocess_HOOK()

dcam’s picture

dcam commented
Status: Needs review » Needs work

I'm glad there was an agreement on no additional changes because this was already annoying with all of the unnecessary array reformatting. At least they were all minor differences. But having to parse additional code changes when comparing the diffs of two 1600 line files would be rough to review.

I left a bunch of comments, mostly about lingering references. But I didn't see anything wrong with the current changes.

BTW, let me know if there would be a better way to note problems like this than what I've done here.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I removed the function.

The comment cleanup should be a follow up I think.

dcam’s picture

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

We discussed in Slack to not edit the docblocks. The other feedback was addressed. It looks good to me.

For the record, @smustgrave is working on the deprecation removal in #3575462: Remove deprecations from themes and misc.

dcam’s picture

dcam commented
Status: Reviewed & tested by the community » Needs work

#3575462: Remove deprecations from themes and misc got committed and there's a conflict for this MR now.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I rebased, since it was just a deprecation removal I just removed that from the output.

dcam’s picture

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

The deleted hook was removed from the MR. Back to RTBC.

sivaji_ganesh_jojodae made their first commit to this issue’s fork.

nicxvan’s picture

nicxvan commented

@sivaji, please do not push any commits to this issue, it uses rector so it's more complex than a usual issue, that is why it is assigned to me.

Thank you for your reviews on all of the other issues!

  • catch committed 24411d2f on main 
    task: #3575584 Convert Claro to OOP

By: nicxvan
By: berdir
By: dcam
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)

Committed/pushed to main, thanks!

Looks like this needs an 11.x backport MR.

nicxvan opened merge request !15104

nicxvan’s picture

nicxvan commented

Ok what I did was run git apply --reject --whitespace=fix patches/claro.patch

This got all changes except the use statement and hook deletions in claro.theme I manually deleted those since it is one block.

Then I looked at the two conflicts:
https://git.drupalcode.org/project/drupal/-/commit/49c2dc169308eb2789929...
and
https://git.drupalcode.org/project/drupal/-/commit/4f51b64b77bb0c42e8d56...

The first I didn't replicate since it's wrong and it was corrected on the main branch already.

The second I manually created the OOP hook here: https://git.drupalcode.org/project/drupal/-/merge_requests/15104/diffs?c...

nicxvan closed merge request !14901

nicxvan’s picture

nicxvan commented
Status: Patch (to be ported) » Needs review
dcam’s picture

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

I figured that the easiest way to review this is to diff the diffs of the two MRs.

They are nearly identical. There are only two differences:

  • A reference to a procedural preprocess function that was updated to the OOP function in main
  • The deprecated preprocess_block_content_add_list() function was moved to ClaroHooks whereas it was removed in main.

I then compared the moved preprocess_block_content_add_list() function to the original version. It was copied faithfully to the hooks class. There's nothing to say about it.

So MR 15104 is nearly the same as the changes that went into main. LGTM.

  • catch committed 3c97b060 on 11.x 
    task: #3575584 Convert Claro to OOP

By: nicxvan
By: berdir
By: dcam
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.

catch closed merge request !15104

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented

Follow up issue https://www.drupal.org/project/drupal/issues/3579925

Status: Fixed » Closed (fixed)

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