Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core

Created on 29 October 2023, about 1 year ago
Updated 12 August 2024, 4 months ago

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.

Proposed resolution

We can't get rid off all implementations and will need to fork those into core ( #3392616-8: Update to Symfony 6.4 β†’ and #3392616-20: Update to Symfony 6.4 β†’ )

However, we want to get rid of as many usage as possible, for that there is πŸ“Œ [META] Reduce use of ContainerAware classes where possible Active .

MR!5181 is the actual MR, but since that also removes all the deprecation suppression it currently fails (hard...).

However if we put the MRs of the children of πŸ“Œ [META] Reduce use of ContainerAware classes where possible Active on top of that MR, as done in MR!5182, we are fully green.

Remaining tasks

Postponed on πŸ“Œ Convert both BookNavigationCacheContext and MenuActiveTrailsCacheContext to use lazy services RTBC
Review

User interface changes

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Closed: won't fix

Version

11.0 πŸ”₯

Component
BaseΒ  β†’

Last updated 2 days ago

Created by

πŸ‡³πŸ‡±Netherlands spokje

Live updates comments and jobs are added and updated live.
  • Needs architectural review

    The issue is available for high level reviews only. If there is a patch or MR it is not to be set to 'Needs work' for coding standards, minor or nit-pick changes.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @spokje
  • @spokje opened merge request.
  • Assigned to spokje
  • Status changed to Needs work about 1 year ago
  • πŸ‡³πŸ‡±Netherlands spokje

    Not quite sure what to do, the actual status would be PP-2, since this is waiting on the commit of both children of πŸ“Œ [META] Reduce use of ContainerAware classes where possible Active to get rid of all the deprecation warnings.

    But maybe someone wants to review the current state, so leave it on NW on my name for now.

  • Status changed to Needs review about 1 year ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Adding what this is postponed on to the remaining tasks per Remining tasks β†’ .

    Changing parent to the S7 compatibility issue, so we find all the related issues.

    But maybe someone wants to review the current state, so leave it on NW on my name for now.

    Hmm. This looks like a situation where the new tag "Needs architectural review" can help. Adding tag and setting to NR.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    The event dispatcher just receives the entire container as an argument:

      event_dispatcher:
        class: Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher
        arguments: ['@service_container']
    

    Can we just do this for the services that need it instead of copying the ContainerAware feature?

  • Issue was unassigned.
  • πŸ‡³πŸ‡±Netherlands spokje

    Can we just do this for the services that need it instead of copying the ContainerAware feature?

    Since I have no clue what this means, I'm quickly un-assigning myself so somebody else can pick this up.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    It just means injecting the whole container as a service via a constructor argument, instead of using the ContainerAwareTrait/Interface helpers.

  • Status changed to Needs work about 1 year ago
  • 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 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.

  • Pipeline finished with Success
    about 1 year ago
    Total: 829s
    #52050
  • Pipeline finished with Failed
    about 1 year ago
    Total: 155s
    #52052
  • Pipeline finished with Failed
    about 1 year ago
    Total: 704s
    #52054
  • Pipeline finished with Success
    about 1 year ago
    Total: 630s
    #52059
  • Pipeline finished with Success
    about 1 year ago
    Total: 898s
    #52063
  • Pipeline finished with Failed
    about 1 year ago
    Total: 670s
    #52073
  • Pipeline finished with Success
    about 1 year ago
    Total: 668s
    #52078
  • Pipeline finished with Success
    about 1 year ago
    Total: 727s
    #52082
  • Pipeline finished with Failed
    about 1 year ago
    Total: 163s
    #52087
  • Pipeline finished with Failed
    about 1 year ago
    Total: 657s
    #52088
  • Pipeline finished with Success
    about 1 year ago
    Total: 1273s
    #52096
  • Pipeline finished with Canceled
    about 1 year ago
    #52110
  • Pipeline finished with Success
    about 1 year ago
    Total: 738s
    #52111
  • Status changed to Needs review about 1 year ago
  • πŸ‡³πŸ‡±Netherlands spokje

    This is the best I could do. I think I've changed all non-factory services to use the container in their constructor and deprecated all the (previously inherited) setContainer methods.

  • Pipeline finished with Failed
    about 1 year ago
    Total: 556s
    #52564
  • Pipeline finished with Success
    about 1 year ago
    Total: 638s
    #52574
  • Pipeline finished with Canceled
    about 1 year ago
    #52594
  • Pipeline finished with Failed
    about 1 year ago
    Total: 152s
    #52595
  • Pipeline finished with Failed
    about 1 year ago
    Total: 491s
    #52611
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 2683s
    #52623
  • Pipeline finished with Success
    about 1 year ago
    #52662
  • πŸ‡³πŸ‡±Netherlands spokje

    Started working on some Factories, but I fear these will not work with constructors having a service container argument, since the container might not be ready yet?

    It passes tests, but I would like some confirmation from somebody who actually knows what she/he is doing before continuing.

  • πŸ‡«πŸ‡·France andypost

    As I see in the MR5181 is the trait is removed but not replaced with fork, so summary needs update

    Issue title said we gonna fork but MR doing replacement which is better to do as rector's rule to be reusable for contrib/custom code

    IMO fixing just a namespace is preferable way, with follow-up to remove usage as the change very disruptive

  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Believe #13 is enough for NW? If not feel free to put back.

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    I've already hit this as in our project we're rejecting code triggering deprecations. It happened when some dependency updated Symfony from 6.3.5 to 6.4.1. Not sure whether to apply this patch or to pin Symfony

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Here's a small patch to remove the deprecation from Symfony because you can't pin to 6.3.5 and use Drupal 10.2.1

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    So it looks like in many of these cases we should be using service locators instead of injecting the entire container.

    CacheFactory, CacheTagsInvalidator, CheckProvider, StreamWrapperManager, KernelDestructionSubscriber all look like they could use this technique.

    EntityTypeManager and ClassResolver are more tricky and need access to the entire container, but I think we could just inject the container as a service in these cases.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Actually, KernelDestructionSubscriber can't use this because a service locator is a separate mini-container, and the destructor needs to know if each individual service was actually used in the main container.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Opened πŸ“Œ Convert KernelDestructionSubscriber to use configurators Active for a slightly hacky way of dealing with KernelDestructionSubscriber.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Opened πŸ“Œ Convert StreamWrapperManager to use a service locator Needs review to try out using a service locator in StreamWrapperManager.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    πŸ“Œ Allow needs_destruction services to run on page cache hits Needs review removes KernelDestructionSubscriber entirely, so πŸ“Œ Convert KernelDestructionSubscriber to use configurators Active can be closed if that lands.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Opened πŸ“Œ Notify downstream users that ContainerAware is going away Needs review which would mean this can be closed as won't fix.

  • Status changed to Closed: won't fix 9 months ago
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    πŸ“Œ Notify downstream users that ContainerAware is going away Needs review landed and we should be able to remove ContainerAware support entirely from Drupal 11 in πŸ“Œ [PP-2] Remove support for ContainerAwareInterface Postponed .

Production build 0.71.5 2024