Problem/Motivation

Since symfony/http-foundation 5.3: "Symfony\Component\HttpFoundation\RequestStack::getMasterRequest()" is deprecated, use "getMainRequest()" instead.

Steps to reproduce

Update to Symfony 5.3 as in #3161889: [META] Symfony 6 compatibility

Proposed resolution

Extend RequestStack and implement a forward-compatibility shim?

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3209617

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Extend RequestStack and implement a forward-compatibility shim?

That sounds good.

longwave opened merge request !578

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review

Opened an MR.

I think we also need a followup to rename StackedRouteMatchInterface::getMasterRouteMatch() to ::getMainRouteMatch().

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Needs work

Marking needs work for re-implementing the deprecation of ::isMasterRequest().

daffie’s picture

daffie CreditAttribution: daffie commented
Related issues: +#3209618: [Symfony 6] Symfony\Component\HttpKernel\Event\KernelEvent::isMasterRequest() is deprecated, use isMainRequest() instead

I think we can use the solution from #3209618: [Symfony 6] Symfony\Component\HttpKernel\Event\KernelEvent::isMasterRequest() is deprecated, use isMainRequest() instead to fix this issue.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I think we could use class_alias() but not sure we need to - if we subclass RequestStack it means we only have to implement the methods we're adding/changing rather than everything on there.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review

Added getMasterRequest() with deprecation message, added SF 5.3 compatibility layer (so we won't trigger deprecations once we are on SF 5.3), fixed tests.

ramankanth’s picture

ramankanth CreditAttribution: ramankanth at QED42 for Drupal India Association commented
ramankanth’s picture

ramankanth CreditAttribution: ramankanth at QED42 for Drupal India Association commented
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

The MR is missing a deprecation message test.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review

Reworked deprecation to use Drupal style as the Symfony style is confusing when we don't actually require SF 5.3 yet, and added a unit test.

daffie’s picture

daffie CreditAttribution: daffie commented
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

The MR is extending the Symfony request stack and in core.services.yml is the new class replacing the old class.
The old method has been deprecated and there is a deprecation message test.
There is no CR, because it is not a "REAL" Drupal deprecation.
For me it is RTBC.
All Symfony 6 issues are critical.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

#12 looks great, don't think there's anything left to do here, so +1 to the RTBC.

  • catch committed 1e3b492 on 9.3.x 
    Issue #3209617 by longwave, daffie, catch: [Symfony 6] Symfony\Component...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x, thanks!

Since this adds a new deprecation and there's no hurry, leaving it out of 9.2

Status: Fixed » Closed (fixed)

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

joachim’s picture

joachim CreditAttribution: joachim at Factorial GmbH commented

Doesn't this need to set a new minimum version of symfony/http-foundation?

`$ composer why -t symfony/http-foundation` says:

symfony/http-foundation v4.4.30 Defines an object-oriented layer for the HTTP specification
├──asm89/stack-cors 1.3.0 (requires symfony/http-foundation ~2.7|~3.0|~4.0|~5.0)
│  └──drupal/core 9.3.x-dev (requires asm89/stack-cors ^1.1)

which means that my D9.3 site's composer is quite happy with Symfony 4, which obviously then crashes because RequestStack::getMainRequest() doesn't exist.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

This caused some contrib breakage #3253542: FacetBlockAjaxController breaks the RequestStack in Drupal 9.3.0. We might want to update the CR to incorporate that. I don't think there's much we could have done to pre-empt it except to have core check method_exists() and fall back to the older method if something is swapping out the service.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

We're going to need to keep this shim in Drupal 10, and deprecate it for removal in Drupal 11 - since modules like facets may start using it in the meantime to bridge versions. Have added a change record now.

Daniel Kulbe’s picture

Daniel Kulbe
he/him
Primary language German
Location Berlin
CreditAttribution: Daniel Kulbe as a volunteer commented

The typehint in Drupal::requestStack() is not correct since this change.