Problem/Motivation

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

Convert template_preprocess in the following modules:

* comment
* filter
* link
* layout_discovery
* media
* media_library
* navigation
* toolbar
* user

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3547806

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

berdir’s picture

Assigned: Unassigned » berdir

berdir’s picture

Issue summary: View changes
Status: Active » Needs review
nicxvan’s picture

Status: Needs review » Needs work

I think that is a real failure

mstrelan’s picture

The test fails because its expecting template_preprocess_layout to exist in $theme_definitions['test_layout_theme']['preprocess functions']. This doesn't exist because we use initial preprocess now instead. There is a snippet in \Drupal\Core\Theme\Registry::processExtension that is responsible here:

// Add template_preprocess_HOOK function, if no initial preprocess
// callback is defined.
if (empty($info['initial preprocess']) && function_exists('template_preprocess_' . $hook)) {
  // @todo trigger deprecation in https://www.drupal.org/project/drupal/issues/3513595.
  $info['preprocess functions'][] = 'template_preprocess_' . $hook;
}

I suspect we don't need testThemeProvidedLayout anymore, or we need something else to verify initial preprocess is set or invoked, specifically for layouts in themes.

mstrelan’s picture

Status: Needs work » Needs review

I must have missed it in debugging, but initial preprocess is right there for us to inspect. Pushed a fix for this.

nicxvan’s picture

Looks right!

I did my normal check except the zebra check.

All deprecations are right
No double __
They all set __FUNCTION__

All initial preprocess are for the correct method.
All comments are correct.
I'll do the zebra check later before marking this.

berdir’s picture

> I must have missed it in debugging, but initial preprocess is right there for us to inspect. Pushed a fix for this.

It's right there, how useful the assert really is is another question I guess. I already updated a unit test that asserts the output of that method, seems rather unnecessary as there's no actual logic/discovery involved with this, but it also doesn't hurt.

nicxvan’s picture

Status: Needs review » Reviewed & tested by the community

Had a chance to do the zebra check looks good to me!
Between this and 9 I think we're good to go on this.

  • catch committed 95324fe1 on 11.x
    Issue #3547806 by berdir, nicxvan, mstrelan: Convert remaining...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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