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.

Drupal\Core\StackMiddleware\Session is container aware because of the following comment:

 * Note, the session service is not injected into this class in order to prevent
 * premature initialization of session storage (database). Instead the session
 * service is retrieved from the container only when handling the request.

Steps to reproduce

Proposed resolution

Wrap the session service in a closure to ensure it is only instantiated when necessary.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3420215

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.

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

longwave opened merge request !6516

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We can't use !service_closure syntax in services.yml because the YAML parser does not support it: #3108309: Support Yaml::PARSE_CUSTOM_TAGS in \Drupal\Component\Serialization\YamlSymfony::decode

However we can enable autowiring instead and use the #[AutowireServiceClosure] attribute.

Unsure of the BC implications here especially as session_manager is backend_overridable.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Functional tests are not very happy.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

session_handler needs to be explicitly autowired.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Not sure I'm taking the right approach here. Maybe we just inject a session closure directly into the session middleware?

longwave opened merge request !6822

longwave changed the visibility of the branch 3420215-remove-containerawaretrait-from to hidden.

longwave closed merge request !6516

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

This simpler approach seems to work. Unsure if this needs BC - are middleware services part of the API?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue summary: View changes
Status: Needs review » Needs work
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
catch’s picture

catch
he/him
Primary language English
commented

are middleware services part of the API?

I'd put them in the same bracket as event subscribers and tagged services (and hook implementations) - so @internal.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

I'd put them in the same bracket as event subscribers and tagged services (and hook implementations) - so @internal.

We might want to documen that here: https://www.drupal.org/about/core/policies/core-change-policies/bc-policy

If we do treat it like @internal, I would say this is RTBC.

catch’s picture

catch
he/him
Primary language English
commented

Since it's just adding another concrete example to an e.g., made the edit: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...

spokje’s picture

spokje
Primary language Dutch
commented

Thanks @catch
_hats were tipped_

  • catch committed 805973ee on 10.3.x 
    Issue #3420215 by longwave, catch, Spokje: Remove ContainerAwareTrait...

  • catch committed b1d0f64f on 11.x 
    Issue #3420215 by longwave, catch, Spokje: Remove ContainerAwareTrait...
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

The service closure usage is nice and will help with issues like #3419693: Log error instead of trigger_error in Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl, and maybe removing our own lazy/proxy services stuff too.

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

Status: Fixed » Closed (fixed)

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