Problem/Motivation

When the language switcher block is rendered for 404 pages an interesting thing happens. Without big_pipe enabled we get links to system/404 - so we get links to pages which are a 200 response but with 404 content. With big_pipe enabled and a logged-in user we get links to the front page. This is because only the sub-request has the correct routing information.

Steps to reproduce

Once #3348592: [regression] Language switcher block throws exception when no route is matched run \Drupal\Tests\language\Functional\LanguageSwitchingTest::testLanguageBlock()

Proposed resolution

Not sure. But the fact that system/401, system/403, system/404 and system/4xx are all valid URLs on a Drupal site is very odd. If you hit them directly they all return a 200 response.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 3349201.patch6.38 KBcatch
#3 interdiff.txt1.78 KBcatch

Issue fork drupal-3349201

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

alexpott created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented

I tried to fix big_pipe via #3348592-22: [regression] Language switcher block throws exception when no route is matched and failed, but it illustrates the problem:

The 404 and 403 patches are subrequests to a route, that we allow to be configured by path in the admin UI. This was done to mirror Drupal 7's settings which also allowed a URL (but directly executed the controller without a subrequest or anything like that iirc).

This affects big_pipe, because placeholders are executed outside the subrequest context, so they don't know have the context of the 404 or 403 page.

The patch I wrote forces the main request to have the routing information for the 404 or 403 page, but this pollutes links with system/404 and similar, which we don't want.

A possible alternative would be to directly execute the controller (i.e. revert even closer to what Drupal 7 does) without any subrequest at all.

If that doesn't work, another possibility might be to allow admins to specify an entity, and modules to override the behaviour programmatically.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new interdiff.txt1.78 KB
new 3349201.patch6.38 KB

Here's a variation of #22 which only swaps the routing information for the main request if it doesn't already have some.

For me this fixes Drupal\Tests\system\Functional\System\AccessDeniedTest

Status: Needs review » Needs work

The last submitted patch, 3: 3349201.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

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

This appears to be caused by #2613044: Requests are pushed onto the request stack twice, popped once and fixing that fixes this!

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Postponed

Postponing on #2613044: Requests are pushed onto the request stack twice, popped once

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

nicxvan’s picture

nicxvan commented
Status: Postponed » Needs review
Related issues: +#2613044: Requests are pushed onto the request stack twice, popped once

#2613044: Requests are pushed onto the request stack twice, popped once is in, can this be closed now?

alexpott’s picture

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

So this has been fixed - but I think this was fixed by a different issue.

Note we still have problems with big_pipe and routing - see #3565887: Null is used as an offset in task and action managers of the menu system. For me it feels as though big pipe should have special request handing for 4xx requests and somehow that big pipe request should have the same request context as the sub request.

alexpott’s picture

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

Yeah this was fixed by #3554414: Use #placeholder_strategy_denylist for CacheOptionalInterface blocks to prevent them being rendered by BigPipe which means that big_pipe is not rendering the language switcher block.

alexpott’s picture

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

And yeah if I remove the fix from that issue and allow the switcher block to be placeholdered by big_pipe everything is still broken - so #6 was wrong.

smustgrave’s picture

smustgrave commented

Free to mark it as a duplicate or outdated?

alexpott opened merge request !15077

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
catch’s picture

catch
he/him
Primary language English
commented

Preventing big pipe from placeholdering in subrequests makes a lot of sense to me. It would good to separately look at reducing our reliance on subrequests for error handling since that causes us a lot of other (usually more serious) problems.

andypost’s picture

andypost
he/him
Primary language Russian
commented

is this changes in test is enough for coverage?

alexpott’s picture

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

@andypost well I think so - this disabling bigpipe is well tested and this does just that.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

I think this makes sense. I was wondering why this isn't using the more explicit isMainRequest(), but I understand that this is only available
on the relevant events, not the request object.

There is a warning on getParentRequest():

* Be warned that making your code aware of the parent request
* might make it un-compatible with other features of your framework
* like ESI support.

I think that isn't a concern here, we aren't really doing anything with the parent request, just check if there is one, and I don't think we support ESI or other main/sub request features that would be relevant here. If anything, the same logic likely applies in such cases too.

While #2613044: Requests are pushed onto the request stack twice, popped once has not fixed this, my understanding is that it made this fix possible, because before that, there would always have been a parent request. That means even though this is a bugfix, we can't backport it to 11.3 as that I think would completely disable bigpipe.

alexpott’s picture

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

@berdir you are completely correct that #2613044: Requests are pushed onto the request stack twice, popped once enables this fix :) I should have pointed that out.

catch closed merge request !15077

  • catch committed ca4517a6 on 11.x 
    fix: #3349201 Big Pipe, logged-in users and 4xx pages

By: catch
By:...

  • catch committed 20efadbb on main 
    fix: #3349201 Big Pipe, logged-in users and 4xx pages

By: catch
By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

Not the most serious of actual bugs, but annoying and disconcerting.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

solideogloria’s picture

solideogloria commented

I see this was fixed in main and 11.x, but I don't see the fix in 11.3.9, despite there being a release newer than the commit date. Why is that?

nicxvan’s picture

nicxvan commented

11.x is 11.4 at this point which hasn't come out yet.

This wasn't backported to 11.3.x so wouldn't be in that release.

solideogloria’s picture

solideogloria commented

Okay, thanks. I can wait.