Problem/Motivation

See #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3518822

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:

Comments

berdir created an issue. See original summary.

berdir’s picture

Title: Convert Convert Template Preprocess hooks in core/includes » Convert template preprocess hooks in core/includes

.

berdir’s picture

Title: Convert template preprocess hooks in core/includes » Convert template preprocess hooks in core/includes/theme.inc
Status: Active » Needs review

This now moves everything in theme.inc, but it's a lot: "115 files + 1291 − 775" (the template references add a lot of files, and the BC layer duplicated documention results in a lot of additional lines being added but that's mostly temporary)

I kept pager in a separate commit so far (has its own pretty large unit test that requires a lot of changes and it's a lot of code) and field preprocess as well (also a lot of code, references from other preprocess functions and a internal but used sort callback, see MR review).

My proposal would be to split those two up into separate issues. They will all conflict due to uses in theme.inc and ThemeCommonElements, but not much more I think.

There's also a question about internal vs final, especially those generic template preprocess functions like table and container are fairly frequently used in contrib. For me, being able to have them final and not worry about constructor BC is much more helpful then the public API of them, about the only thing that could happen is that we deprecate a template I think, then we could just deprecate it and let it be there.

smustgrave’s picture

it is a lot, even though it's shuffling things around would it be worth breaking up?

berdir’s picture

Yes, as mentioned in #4, I've now split off #3519257: Convert template_preprocess_pager() and #3519259: Convert field template_preprocess functions in theme.inc now. I did add authorize report, it's trivial with only two uses and no dependencies and it helps #1177762: Deprecate automatic template_preprocess discovery and 'includes'/'file' support from hook_theme()

That removes the two most complex cases and about a third of the size off this one.

I did my best to not include any changes except DI and required reflowing of some comments due to the changed length limits due to them being two spaces more nested. Try using git diff 11.x --color-moved=dimmed_zebra --color-moved-ws=ignore-all-space` to review these issues. It should mark all lines that were just moved and not changed otherwise grey. Doesn't work for the docblocks because those are copied and there don't seem to be any options to highlight that.

smustgrave’s picture

Status: Needs review » Needs work

This one appears to have pipeline issues.

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

berdir’s picture

Status: Needs work » Needs review

Pipelines have issues, that's not the fault of this issue, unit tests not passing only on 8.4 is _extremely_ unlikely to be a real issue.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

dww’s picture

berdir’s picture

Soft blocked on #3519257: Convert template_preprocess_pager(), will rebase then to avoid further conflicts with that.

berdir’s picture

Status: Needs work » Needs review

Rebased this as pager got in.

I skipped authorize_report as that's already deprecated now. We might want to move that into theme.inc in to so we can deprecate the file key, but no point in moving it to a service now.

Reminder: --color-moved is really useful to review this one, see https://www.md-systems.ch/en/blog/techblog/2011/08/22/git-aliases. all lines that are grey can be mostly ignored then as they haven't changed.

Added template_preprocess_tablesort_indicator() as that was recently added.

Also noticed that template_preprocess_pager() got in with inconsistent deprecation version number, fixed on 11.2 there.

nicxvan’s picture

Status: Needs review » Reviewed & tested by the community

This is ready!

I ran git diff 11.x --color-moved=dimmed_zebra --color-moved-ws=ignore-all-space
I've attached an example of what it looks like.
This is what makes the slightly higher than normal line count change easy to review.
It grays out lines that were just copied to a new file with no change so you are able to review the actual changes like the method names, comments, DI, and anything else.

I confirmed each deprecation was 11.3 -> 12.0
I confirmed each was __FUNCTION__
I confirmed the service called matched the template function
I confirmed the comments updated from the correct template function to the new location.

The class split makes sense to me.
I checked the changes were all around line length, t() and DI.
There was a test or two that directly called the template function that were updated.

I checked in the MR and here that the hook theme updates set the right location for each initial preprocess.

nicxvan’s picture

StatusFileSize
new62.34 KB

Attached the screenshot to show an example of the output and how it's easy to see what changed.

git output

MR

If you compare these changes you can see in the first some lines were changed when copied to the method.
The second screenshot shows those changes in the MR so you can see that while the highlighted lines changed the changed code is equivalent.

nicxvan’s picture

StatusFileSize
new67.02 KB

Upload the corresponding change.

  • catch committed 1e79f5be on 11.x
    Issue #3518822 by berdir, nicxvan: Convert template preprocess hooks in...
catch’s picture

Status: Reviewed & tested by the community » Fixed
 git diff 11.x --color-moved=dimmed_zebra --color-moved-ws=ignore-all-space

This is a new one on me, very nice!

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

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