Problem/Motivation

@berdir pointed out that I forgot to leave a couple of test hooks for themes to ensure we don't break procedural hooks.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3579629

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
nicxvan’s picture

nicxvan commented

Still need to add preprocess and preprocess_HOOK, but got a super simple kernel test for checking collecting and executing procedural hooks in themes.

nicxvan opened merge request !15130

nicxvan’s picture

nicxvan commented
Status: Active » Needs review

Added some fairly simple tests to confirm that procedural hooks and preprocess hooks are picked up.

I also added a test to confirm LegacyHook works as expected.

nicxvan’s picture

nicxvan commented
Priority: Major » Normal
Issue summary: View changes

Moving to normal because there are a couple of procedural hooks still, but we really should have explicit coverage since the remaining ones are being converted.

nicxvan’s picture

nicxvan commented
Parent issue: » #3554670: [meta] Eliminate core .theme files
berdir’s picture

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

> Moving to normal because there are a couple of procedural hooks still, but we really should have explicit coverage since the remaining ones are being converted.

True, but they will vanish quick enough.

Added some minor remarks, once we deprecate having .theme files explicitly we can then just add the necessary expected deprecations here.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Yep, I would bump the priority I think if those get in first.

I addressed your feedback, thank you.

nicxvan’s picture

nicxvan commented
Priority: Normal » Major

Other two are in this is major at least now.

berdir’s picture

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

One more thing.

nicxvan’s picture

nicxvan commented

I was thinking we should probably do that too.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

I was not successful in converting it to a kernel test, but I added real rendering. The browser test is real rendering so I think it is actually better coverage.

I think this covers the procedural theme pipeline.

nicxvan’s picture

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

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Per #14, there were some issues trying to do all of this in a kernel test. I'm not sure why, I think it should be possible and I pondered if I should look into it myself. I decided against it, I think it doesn't hurt to have some full functional testing of this and not just kernel tests, wouldn't be the first time that something only worked in a kernel/unit test due to setup that didn't actually match a real size.

This is major because it a) helps us guarantee that the BC layer continues to work until 13.0 and it the tests added here can be converted directly to deprecation tests for the #3581218: Deprecate .theme file extension so that we have tests for that.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

Per #14, there were some issues trying to do all of this in a kernel test. I'm not sure why

Only guessing since I don't know what the issues were, but if you need to use node entities in kernel tests, you need to add both node and user to $modules. See https://www.drupal.org/node/3539877

I think a full functional test is fine though.

Some comments on the MR.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I addressed all of your feedback thanks!

Only guessing since I don't know what the issues were, but if you need to use node entities in kernel tests, you need to add both node and user to $modules

I bet that was it, but I think functional still provides better coverage here.

nicxvan’s picture

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

Fixed the last comment, @godotislate mentioned in slack I could self RTBC once it was green again.

godotislate closed merge request !15130

  • godotislate committed 036963e5 on main 
    test: #3579629 Add procedural hooks to test theme to test collection and...

  • godotislate committed abda4b5a on 11.x 
    test: #3579629 Add procedural hooks to test theme to test collection and...

  • godotislate committed 56a8ed21 on 11.3.x 
    test: #3579629 Add procedural hooks to test theme to test collection and...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 036963e to main, abda4b5 to 11.x, and 56a8ed2 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.