Problem/Motivation

theme.api.php still documents hook_extension() and hook_render_template, a hook that Drupal 11.3.0 made obsolete.
#1685492: Convert theme engines into services

Steps to reproduce

Open core/lib/Drupal/Core/Render/theme.api.php.

Proposed resolution

Remove the references to the two hooks.

Remaining tasks

N/A

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#14 3624887-nr-bot_exgqaktf.txt550 bytesneeds-review-queue-bot
#10 3624887-nr-bot_92zj1mp1.txt550 bytesneeds-review-queue-bot

Issue fork drupal-3624887

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

matthand created an issue. See original summary.

matthand’s picture

matthand
he/him
Primary language English
Location Maryland
commented
Related issues: +#3624888: Remove hook_render_template(), dead documentation left behind by the 11.3 theme engine service conversion
nicxvan’s picture

nicxvan commented
Issue tags: -good first issue +Needs issue summary update

Good find!

I think we can clean up both of those in one issue here.

I don't think we need quite that much detail in the IS it amounts to update documentation.

I'm going to close the other issue.

nicxvan’s picture

nicxvan commented
Title: Remove hook_extension(), dead documentation left behind by the 11.3 theme engine service conversion » Remove obsolete hook_extension() and documentation
nicxvan’s picture

nicxvan commented
Title: Remove obsolete hook_extension() and documentation » Remove obsolete hook_extension() and hook_render_template() documentation
sapnil_biswas’s picture

sapnil_biswas commented
Assigned: Unassigned » sapnil_biswas

Starting to work on this!

sapnil_biswas opened merge request !17222

sapnil_biswas’s picture

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

nicxvan commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs issue summary update +mobile-novice

Looks good to me!

I updated the Issue Summary as well.

This kind of change doesn't need a release note.

I confirmed these were the only two remaining references to the hook, and it is not executed any longer either.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3624887-nr-bot_92zj1mp1.txt550 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

sapnil_biswas’s picture

sapnil_biswas commented

@nicxvan Just to clarify one thing – the Needs Review Queue Bot changed the issue status to “Needs work” because it failed Drupal core commit checks. You said the changes look good and confirmed that these are the only remaining references to the hook. Does this status need any action from my side or can be ignored/updated accordingly? Thanks!

nicxvan’s picture

nicxvan commented

It might need a rebase, if there are no conflicts with the rebase you should be good to reset it to RTBC yourself.

sapnil_biswas’s picture

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

The latest pipeline failed on ThemeSettingsFormTest, but it's an unrelated random failure since this patch only removes documentation from theme.api.php. Re-setting to RTBC!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3624887-nr-bot_exgqaktf.txt550 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

sapnil_biswas’s picture

sapnil_biswas commented

@nicxvan The bot always marks this as needs work. Is there any issue with this or something?

nicxvan’s picture

nicxvan commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +no-needs-review-bot

I'm not sure why it's kicking it back, I think sometimes doc only changes confuse it.

We can add the no-needs-review-bot tag to stop it from changing status again.

That tag should rarely be used.

matthand’s picture

matthand
he/him
Primary language English
Location Maryland
commented

I agree with the RTBC here. Nice work @sapnil_biswas! I reviewed the MR as well and it correctly removes the references to dead code. I also noted there was a random test failure on a Theme Settings FuncitonalJavascript test that was unrelated to this change. I filed a followup issue for that flaky test #3626907: Random test failures on ThemeSettingsFormTest::testFormSettingsSubmissionHandler.