Problem/Motivation

There were four preprocess hooks missed in bulk conversion because they were not documented.

Steps to reproduce

  • comment_preprocess_field
  • media_library_preprocess_views_view__media_library
  • views_preprocess_node
  • views_preprocess_comment

Proposed resolution

Convert these preprocess to OOP and mark as skip procedural scanning

Remaining tasks

Do it.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3535943

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
Related issues: +#3535675: Remove twig_theme_test.module

nicxvan opened merge request !12720

nicxvan’s picture

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

mstrelan commented

One comment on the @todo item for #939462: Specific preprocess functions for theme hook suggestions are not invoked otherwise looks good.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

One note: If we then indeed converted all module preprocess to OOP then that means we wouldn't have any test coverage of the BC layer for this.

On the plus side, that means we can formally deprecate legacy preprocess functions and as part of that, introduce one example in a test module to ensure that deprecations are triggered correctly, lets make sure we have an issue for that.

nicxvan’s picture

nicxvan commented

Yes I meant to add one to the legacy hook module. But technically there are themes implementing them still.

nicxvan’s picture

nicxvan commented
Related issues: +#3536195: Deprecate procedural preprocess with bc, +#3536196: Investigate preprocess_field in the comment module

Created follow ups.

mstrelan’s picture

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

Confirmed all hooks still working:

  • CommentThemeHooks::preprocessField invoked in \Drupal\Tests\comment\Functional\CommentNonNodeTest
  • MediaLibraryThemeHooks::preprocessViewsViewMediaLibrary is invoked in MediaLibraryTestBase::assertMediaLibraryGrid
  • ViewsThemeHooks::preprocessNode is invoked in \Drupal\Tests\views\Kernel\Entity\RowEntityRenderersTest::testEntityRenderers
  • ViewsThemeHooks::preprocessComment is invoked in \Drupal\Tests\comment\Functional\CommentRssTest::testCommentRss
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Agreed with the two follow-ups, but there is one outdated comment I think we should delete here. Would have done it in gitlab suggestions but it's not letting me delete those three lines in a suggestion.

nicxvan’s picture

nicxvan commented

Removed that comment block!

FYI if there is a comment on a line you can't do a multiline suggestion by dragging the comment icon.

You can either do two suggestions, one for the line with the comment and a single for the other lines.

Or you can click the suggestion then change the line numbers in the comment, but that gets tricky, I think desktop will update to show you what you are editing, but phone won't.

Either way I don't mind making the change myself, just sharing some gitlab tips I've discovered.

nicxvan’s picture

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

nicxvan commented

Also removed ProceduralHookScanStop here for media module as @berdir pointed out here: #3494908: Set skip procedural scanning for all modules in core

mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

Think this is back to NW for @berdir's latest comment about the ProceduralHookScanStop

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Nope, I took care of that this morning, ready for review!

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Oh in the mr! Sorry you're right!

nicxvan’s picture

nicxvan commented

I undid the media changes

mstrelan’s picture

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

Think this is good to go. Failing test is unrelated.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

  • catch committed fc8732f2 on 11.x 
    Issue #3535943 by nicxvan, mstrelan, berdir: Convert final 4 preprocess...
catch’s picture

catch
he/him
Primary language English
commented

catch closed merge request !12720

Status: Fixed » Closed (fixed)

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