Problem/Motivation

From https://github.com/symfony/http-kernel/blob/5.x/HttpKernelInterface.php#...

    /**
     * @deprecated since symfony/http-kernel 5.3, use MAIN_REQUEST instead.
     *             To ease the migration, this constant won't be removed until Symfony 7.0.
     */
    public const MASTER_REQUEST = self::MAIN_REQUEST;

Steps to reproduce

Proposed resolution

Add an FC (forward compatibility) layer, replace the constant.

Search for related instances of "master" in comments and replace them with "main" at the same time.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#22 interdiff_20-22.txt3.07 KBmurilohp
#22 3209723-22.patch37.82 KBmurilohp
#20 3209723-20.patch35.03 KBmurilohp
#19 reroll_diff-16-19.txt5.49 KBmurilohp
#19 3209723-19.patch35.18 KBmurilohp
#16 reroll_diff_7-16.txt18.71 KBmurilohp
#16 3209723-16.patch37.45 KBmurilohp
#7 3209723-7.patch37.21 KBdaffie
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

I think we can take the same approach here as we have in #3209618: [Symfony 6] Symfony\Component\HttpKernel\Event\KernelEvent::isMasterRequest() is deprecated, use isMainRequest() instead.

However also noting this:

To ease the migration, this constant won't be removed until Symfony 7.0.

If we just suppressed the deprecation message, then both core and modules could switch over during Drupal 10, the only issue then is that Drupal 9 + Drupal 11 compatibility wouldn't be possible.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

I don't think there will be a deprecation message, as this is just a constant and so there is no code executed when it is used - this is why they are keeping it around until Symfony 7 I guess. I think we can probably just leave this one until the D10 cycle, there seems no pressing need to do it now with an FC layer.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Checked usage and only Kernel::handle() with middlewares affected but 2/3 of (50) in tests could be easy replaced

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.

daffie’s picture

daffie CreditAttribution: daffie commented
Version: 9.3.x-dev » 10.0.x-dev
Priority: Normal » Critical
Status: Active » Needs review
FileSize
3209723-7.patch37.21 KB

The problem with this patch is that the constant is used as the default value for the paramter $type in the method Drupal\Core\DrupalKernel::handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE). Therefor we cannot replace the constant in a minor version release, it must be done in a new major release. Moving this to be committed in D10.0.

This issue is part of the get Symfony 6 in D10 and therefor it is a critical issue.

daffie’s picture

daffie CreditAttribution: daffie commented

Added a CR.

catch’s picture

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

Can't we make the change everywhere except Drupal\Core\DrupalKernel::handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE). and open a follow-up for that one method to do in Drupal 10?

On the other hand Symfony itself won't drop the old constant until Symfony 7, so there is not the same urgency here as other Symfony 6 changes.

daffie’s picture

daffie CreditAttribution: daffie commented

On the other hand Symfony itself won't drop the old constant until Symfony 7, so there is not the same urgency here as other Symfony 6 changes.

That is why I wanted to wait for the 10.0 branch to commit this patch.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

@catch we need an FC layer to do that in Drupal 9 as the constant doesn't exist in Symfony 4. IMO this should just be postponed, we're only making work for ourselves trying to fit it in now if we can wait until we are on Symfony 6.

catch’s picture

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

Oh good point we'd have to add a shim, yeah let's leave this for Drupal 10 then.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Postponed
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Postponed » Needs work

Unpostponing as the 10.0.x branch is open.

daffie’s picture

daffie CreditAttribution: daffie commented
Issue tags: +Needs reroll
murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Issue tags: -Needs reroll
FileSize
3209723-16.patch37.45 KB
reroll_diff_7-16.txt18.71 KB

Here's the reroll of patch #7 to branch 10.0.x.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

This still can't go in until at least Symfony 5.4 is committed.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs work » Postponed

Postponing on #3197482: Update Drupal 10 to depend on Symfony 5.4 (as a stepping stone to Symfony 6, for deprecation checking support) as stated in #17

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Status: Postponed » Needs review
FileSize
3209723-19.patch35.18 KB
reroll_diff-16-19.txt5.49 KB

The #3197482 landed! I rerolled #16, let's see now.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
FileSize
3209723-20.patch35.03 KB

Fixing the coding standard errors. I accidentally added a new empty file.

longwave’s picture

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

Nearly there, two cases missing that I can see:

core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
167:  public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, $catch = TRUE): Response;

core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
117:  public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, $catch = TRUE): Response;
murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Status: Needs work » Needs review
FileSize
3209723-22.patch37.82 KB
interdiff_20-22.txt3.07 KB

Thanks for the review @longwave! Addressing #21 on this new patch.

daffie’s picture

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

All code changes look good to me.
I have installed the patch on my local machine and I could not find any more instances of the old constant.
The IS and the CR are in order.
For me it is RTBC.

  • catch committed 7f93a36 on 10.0.x 
    Issue #3209723 by murilohp, daffie, longwave, andypost: [Symfony 6]...
catch’s picture

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

Committed 7f93a36 and pushed to 10.0.x. Thanks!

Current status is that contrib modules won't be able to resolve the Symfony 5/6 deprecation until they drop support for Drupal 9.4, but we'll reach that point before the Drupal 11 branch opens and updates to Symfony 7. If Symfony was dropping this in Symfony 6, we'd need to add a shim to Drupal 9, but they're not.

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Updated and published the CR.