Problem/Motivation

Spin-off from #2976394: Allow Symfony 4.4 to be installed in Drupal 8 which includes both Symfony4 and cors/stack updates.

Core uses Request::getRequestUri() since #2761639: PageCache should not use $request->getUri() to ensure that page cache cache IDs are unique to the actual request URI, however since Symfony 3.4.19 it uses parse_url(), which replaces '?' with '' when the query string is empty.

This means that example.com and example.com? will received the same page cache ID.

Additionally, the Symfony 4 update no longer sends Vary:cookie headers for cached responses, but this is correct behaviour, so the patch just makes the test stop looking for it.

Proposed resolution

Adjust the test coverage to account for the change, but we need to decide if and what to do about this for globalredirect. #3024271: Request::getRequestUri() strips empty query string since 3.4.19 is a follow-up for the original globalredirect issue we added test coverage for.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#23 3021236-22.patch2.87 KBcatch
#21 3021236-symfony-3.patch50.8 KBcatch
#21 3021236-symfony-3-review.patch2.87 KBcatch
#20 3021236-20-combined.patch156.48 KBcatch
#18 3021236-18-review.patch2.87 KBcatch
#18 302123-18-combined.patch163.22 KBcatch
#6 302123-4-review.patch3 KBcatch
#6 302123-4-combined.patch160.47 KBcatch
#3 3021236-review.patch2.88 KBcatch
#2 3021236.patch160.34 KBcatch

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Title: Page cache middleware not adding headers with Symfony 4 » Page cache fails with updated guzzle/symfony 4
Status: Active » Needs review
Parent issue: » #2937984: [META] Symfony 4.0 compatibility
StatusFileSize
new 3021236.patch160.34 KB

OK so it's not actually a problem with the headers, it's real changes in Symfony etc. that break assumptions in the tests.

1. The tests assume that Vary: accept-encoding is always there, but it's now not. We don't explicitly set that anywhere that I could find.

2. Symfony's prepareRequestUri now uses parse_url, which will sort the query string and behave the same for empty vs null - so where we previously tested that differently ordered but otherwise identical query strings would not share a cache item, they now do.

If we decide that current behaviour is what we want, then we'll need to fix this differently, but this patch should allow the tests to pass and shows what's different.

Note that the review patch will fail on the Vary header stuff still - I'll probably work on a version that's more agnostic about what it gets back.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3021236-review.patch2.88 KB

Review patch didn't stick, here it is.

The last submitted patch, 2: 3021236.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 3: 3021236-review.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 302123-4-combined.patch160.47 KB
new 302123-4-review.patch3 KB

This should pass on both Symfony 3 and 4, it doesn't dilute the test strictness too much IMO. Haven't check the page cache tags test yet.

The last submitted patch, 6: 302123-4-combined.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented

The six fails in the combined patch for #6 are in unrelated tests dealt with by #3020536: ControllerResolver::getArguments() is removed in Symfony 4 and ControllerResolver no longer implements ArgumentResolverInterface so this is green for both Symfony 3 and 4 now.

catch’s picture

catch
he/him
Primary language English
commented
Title: Page cache fails with updated guzzle/symfony 4 » Page cache test fails with Symfony 4
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This doesn't seem like it loosens the tests by a lot, it looks like we still test the things we care about.

This might need a review from Wim. But this is very simple and #2 explains the why we're doing this I feel confident in moving this to RTBC.

catch’s picture

catch
he/him
Primary language English
commented

Just coming back to say the test removals (normalized URL) are from #2761639: PageCache should not use $request->getUri(). The idea there was to use $request->getRequestUri() to use the actual, literal URI for the request instead of a normalized one. Symfony is now normalizing some things (query string ordering and empty query strings), but not everything. So for the global redirect in core case, it'd be necessary to find yet another way to get the actual requested Uri if we really wanted to redirect all of these cases - not sure Symfony even offers that any more.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yeah, we need to test this a bit more IMHO.

The problem is that normalizing the URL might result in redirects that are then cached under a different URL. That said, if symfony itself changes, global redirect might also no longer try to redirect or change the URL.

catch’s picture

catch
he/him
Primary language English
commented

Right the only alternative I can think of is persuading Symfony to revert to the old behaviour somehow but haven't looked into exactly what that would take yet.

This is the commit that changed the behaviour, from a month ago: https://github.com/symfony/http-foundation/commit/fbf03645a8cc45aab9ffc7...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

@Berdir re #12 so do you think this needs more work or be a follow-up issue?

The last submitted patch, 6: 302123-4-combined.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Page cache test fails with Symfony 4 » Page cache test fails with Symfony 3.4.19 and above

#13 the same was committed to the 3.4 branch of Symfony, so this issue is even more relevant since now sites can be upgraded to that version based on current composer constraints.

EDIT: you can see test failure at #2976407-40: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I can't work out how the case

      [
        $url . '?z=z&a=a',
        $url . '?a=a&z=z',
      ],

is broken by https://github.com/symfony/http-foundation/commit/fbf03645a8cc45aab9ffc7...

I think that test case is still good. But the 

      [
        $url . '?',
        $url . '',
      ],

Needs removing because the following Symfony code...

            $requestUri = $this->server->get('REQUEST_URI');
            // HTTP proxy reqs setup request URI with scheme and host [and port] + the URL path, only use URL path
            $uriComponents = parse_url($requestUri);
            if (isset($uriComponents['path'])) {
                $requestUri = $uriComponents['path'];
            }
            if (isset($uriComponents['query'])) {
                $requestUri .= '?'.$uriComponents['query'];
            }

Means that a single ? will be removed from the request URI because $uriComponents['query'] will not be set. There is discussion of redirecting a URI's ending with ? - see #2761639-23: PageCache should not use $request->getUri(). And @Berdir has pointed out that the behaviour of removing the ? in problematic for the Redirect module - see #2761639-29: PageCache should not use $request->getUri() - so we're back to that being a bug.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 302123-18-combined.patch163.22 KB
new 3021236-18-review.patch2.87 KB

Here's a patch with that assertion restored, but unless I missed something it will still fail once it's added back. Definitely parse_url() should not be the cause of that failure, but something else must be and I haven't found a candidate yet.

edit: turns out the candidate was me/PEBKAC, that change is completely unnecessary. '?' vs. '' definitely needed though.

The last submitted patch, 18: 302123-18-combined.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3021236-20-combined.patch156.48 KB

Needed a re-roll, review patch is the same.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3021236-symfony-3-review.patch2.87 KB
new 3021236-symfony-3.patch50.8 KB

Annnd a patch that only updates Symfony 3 to show the test failure in this issue.

Status: Needs review » Needs work

The last submitted patch, 21: 3021236-symfony-3.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 3021236-22.patch2.87 KB

OK so the parse_url() fix is the same for Symfony 3.4.19 and Symfony 4, the query string ordering was PEBKAC/red herring.

Here's the actual patch to review by itself because I messed up patch naming/ordering above. I personally think this one is committable - we can open a follow-up to revisit the globalredirect/empty query string issue again.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Since this only restored two lines of test coverage since the RTBC in #10, tentatively moving this back to RTBC.

Opened a follow-up for the globalredirect issue: #3024271: Request::getRequestUri() strips empty query string since 3.4.19 and updated the issue summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

So upgrade to Symfony 4 does indeed break the global redirect tests. Specifically Drupal\\redirect\\Tests\\GlobalRedirectTest breaks on line 182 $this->assertRedirect('test-node?', 'test-node'); - in manual testing the good news is that it doesn't appear to enter a redirect loop. I also tested URLs like ///node/1 @Berdir's suggestion in case they caused a redirect loop.

I couldn't get anything to break so I think we should commit this patch because in itself it doesn't result in the redirect module breaking - we'v got the core follow-up for furtyher discussion and we probably want a further redirect module follow-up to prepare for Symfony4 and handle the empty query string redirect in a different way.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Credited myslef and @Berdir for issue review.

Committed a013f05 and pushed to 8.7.x. Thanks!

  • alexpott committed a013f05 on 8.7.x 
    Issue #3021236 by catch, alexpott, Berdir: Page cache test fails with...

Status: Fixed » Closed (fixed)

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