Problem/Motivation

Drupal\Tests\Core\StackMiddleware\ReverseProxyMiddlewareTest::testReverseProxyEnabledLegacy() is a legacy test that is skipped on Symfony 4 as it is only relevant to Symfony 3 and below. Drupal 9 ships with a minimum of Symfony 4 so the test is obsolete.

Proposed resolution

Remove the test.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3135302.patch3.42 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
FileSize
3135302.patch3.42 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Category: Bug report » Task
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented

Thanks for the patch.

1) Applies cleanly to 8.9, 9.0, and 9.1.

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < 3135302.patch
patching file core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
[mac:kristen:drupal-8.9.x-dev]$ cd90
[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3135302.patch
patching file core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
[mac:kristen:drupal-9.0.x-dev]$ cd91
[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3135302.patch
patching file core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php

2) Reviewed the code being removed.

  1. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
@@ -84,58 +84,6 @@ public function reverseProxyEnabledProvider() {
-      $this->markTestSkipped('The method \Symfony\Component\HttpFoundation\Request::setTrustedHeaderName() does not exist therefore testing on Symfony 4 or greater.');

    Shows skipped for < Symfony 4. 

  2. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
@@ -84,58 +84,6 @@ public function reverseProxyEnabledProvider() {
-          'The \'reverse_proxy_host_header\' setting in settings.php is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the \'reverse_proxy_trusted_headers\' setting instead. See https://www.drupal.org/node/3030558',
-          'The \'reverse_proxy_forwarded_header\' setting in settings.php is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the \'reverse_proxy_trusted_headers\' setting instead. See https://www.drupal.org/node/3030558',
-          'The "Symfony\Component\HttpFoundation\Request::setTrustedHeaderName()" method is deprecated since Symfony 3.3 and will be removed in 4.0. Use the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.',

    Shows removal for 9.0.0 and Symfony 4.0. 

  3. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
@@ -84,58 +84,6 @@ public function reverseProxyEnabledProvider() {
-          'The \'reverse_proxy_forwarded_header\' setting in settings.php is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use the \'reverse_proxy_trusted_headers\' setting instead. See https://www.drupal.org/node/3030558',
-          'The "Symfony\Component\HttpFoundation\Request::setTrustedHeaderName()" method is deprecated since Symfony 3.3 and will be removed in 4.0. Use the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.',

    Same.

3) Tests pass.

4) Seems RTBC to me but general question: should each of these types of removals be handled one by one or should there be a big issue for a bunch or maybe at least a meta issue?

longwave’s picture

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

Thanks for the review :)

#4.1 This should *not* be backported to 8.9 as we are still on Symfony 3 there.

#4.4 I don't really know, I filed a few similar issues but they aren't really related to each other except that they are stragglers left over from the Symfony 4.4 upgrade. Maybe we need a "Remove compatibility shims for Symfony 4.3 and earlier" meta issue to tie them together?

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Reviewed & tested by the community

Gotcha. A meta issue certainly wouldn't hurt. :) Marking this one RTBC. Thanks for the patch and feedback.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Parent issue: » #3136002: [meta] Remove compatibility shims for Symfony 4.3 and earlier

  • xjm committed 39bd419 on 9.1.x 
    Issue #3135302 by longwave, Kristen Pol: Remove Symfony 3 legacy test...

  • xjm committed d5b29b2 on 9.0.x 
    Issue #3135302 by longwave, Kristen Pol: Remove Symfony 3 legacy test...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 9.1.x and cherry-picked to 9.0.x as a D9-only test cleanup. Thanks for the great review and discussion on this issue; made it a lot easier to validate and commit this patch.

Status: Fixed » Closed (fixed)

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