Problem/Motivation

In #3392616-5: Update to Symfony 6.4 we've found out that the Symfony\Component\DependencyInjection\ContainerAwareTrait and Symfony\Component\DependencyInjection\ContainerAwareInterface are being deprecated in Symfony 6.4 and removed in 7.0.

A number of test classes use ContainerAware for testing purposes. We should remove the trait and use dependency injection properly or inject the entire container as required.

Steps to reproduce

Proposed resolution

- Remove usage of ContainerAwareTrait from test classes
- Keep tests affected by ContainerAwareInterface until interface is used in \Drupal\Core\DependencyInjection\ClassResolver::getInstanceFromDefinition() - probably #3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3421482

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

longwave created an issue. See original summary.

spokje’s picture

spokje
Primary language Dutch
commented
Title: Remove ContainerAwareTrait from OpenerResolver » Remove ContainerAwareTrait from test classes

Retitling based on the 2nd paragraph of the IS and the fact that a similar titled issue was created just before this one (Probably copy-pasta?)

andypost made their first commit to this issue’s fork.

andypost opened merge request !6813

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review

Only 3 tests are affected, fixed by removing trait as interface should be tested still.

Changed ControllerResolverTest to be inline with CallableResolverTest

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record

Wonder if we should add a CR for the trait removal.

smustgrave’s picture

smustgrave commented

Probably good to self RTBC as the removal seems fine.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

This is only in tests, no CR needed.

catch’s picture

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

Should the classes that are removing ContainerAwareTrait and implement ContainerAwareInterface also be dropping ContainerAwareInterface here too?

andypost’s picture

andypost
he/him
Primary language Russian
commented

I decided to leave interface as is until we remove it from controller resolver so no code remain untested

andypost’s picture

andypost
he/him
Primary language Russian
commented

Sorry, I mean ClassResolver

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Maybe we just postpone this until we have figured out what to do with ClassResolver?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Related issues: +#3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core

Updated IS with proposal

smustgrave’s picture

smustgrave commented

So is this postponed?

andypost’s picture

andypost
he/him
Primary language Russian
commented

I see no reason as class resolver and its tests should be fixed in other issue but this one is clean-up of useless places

smustgrave’s picture

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

In that case removal seems fine.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

OK yeah let's do this here and decide what to do with the class resolver later.

  • catch committed b7348d1b on 10.3.x 
    Issue #3421482 by andypost, longwave, smustgrave: Remove...

  • catch committed 19baa4e1 on 11.x 
    Issue #3421482 by andypost, longwave, smustgrave: Remove...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

OK fair enough. Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

andypost closed merge request !6813

Status: Fixed » Closed (fixed)

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