Bug #83755

Extbase TYPO3\CMS\Extbase\Mvc\Controller\AbstractController->redirectToUri() – remove HTML redirect?

Added by Hagen Gebauer over 1 year ago. Updated 5 months ago.

Status:
New
Priority:
Should have
Assignee:
-
Category:
Extbase
Start date:
2018-02-02
Due date:
% Done:

100%

TYPO3 Version:
7
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

Maybe there is a point to that which I don’t see … but redirecting should in my opinion always be done with header('Location: ...') and not by the HTML meta tag. I believe that this method does that correctly at its first call – but then the HTML gets cached while response->setStatus() and response->setHeader() do not. Besides the full page HTML will be written into the content part of the template – see this simplified example:

<!DOCTYPE html>
<html>
<head>
    <!-- html header from page/template -->
</head>
<body>
    <div class="content">
        <html><head><meta http-equiv="refresh" content="..."/></head></html>
    </div>
</body>
</html>

Related issues

Related to TYPO3 Core - Bug #88178: Page cache is deleted, when an uncached (Extbase) Plugin performs a redirect Closed 2019-04-19

Associated revisions

Revision 3139b560 (diff)
Added by Markus Klein 12 months ago

[BUGFIX] Do not cache content with different status code than 200

Resolves: #83755
Releases: master, 8.7, 7.6
Change-Id: I6e13133f221137c63283ec1575fc405a38668b1a
Reviewed-on: https://review.typo3.org/58582
Tested-by: TYPO3com <>
Reviewed-by: Andreas Fernandez <>
Reviewed-by: Benni Mack <>
Reviewed-by: Johannes Kasberger <>
Tested-by: Johannes Kasberger <>
Reviewed-by: Tomas Norre Mikkelsen <>
Reviewed-by: Stefan Neufeind <>
Tested-by: Georg Ringer <>

Revision 73122020 (diff)
Added by Markus Klein 12 months ago

[BUGFIX] Do not cache content with different status code than 200

Resolves: #83755
Releases: master, 8.7, 7.6
Change-Id: I6e13133f221137c63283ec1575fc405a38668b1a
Reviewed-on: https://review.typo3.org/58581
Tested-by: TYPO3com <>
Reviewed-by: Markus Klein <>
Tested-by: Markus Klein <>

Revision ad116036 (diff)
Added by Markus Klein 12 months ago

[BUGFIX] Do not cache content with different status code than 200

Resolves: #83755
Releases: master, 8.7, 7.6
Change-Id: I6e13133f221137c63283ec1575fc405a38668b1a
Reviewed-on: https://review.typo3.org/58580
Tested-by: TYPO3com <>
Reviewed-by: Markus Klein <>
Tested-by: Markus Klein <>

History

#1 Updated by Michael Stucki over 1 year ago

  • Tracker changed from Support to Bug
  • Project changed from forge.typo3.org to TYPO3 Core
  • Category deleted (Development)
  • TYPO3 Version set to 9

Wrong project!

#2 Updated by Riccardo De Contardi about 1 year ago

  • Category set to Extbase + l10n

#3 Updated by Markus Klein 12 months ago

  • Status changed from New to Accepted
  • Target version set to Candidate for patchlevel
  • TYPO3 Version changed from 9 to 7

Valid from v7 through v9

#4 Updated by Markus Klein 12 months ago

My take on this issue:

The first plugin on a page with a redirect wins. The request can be terminated immediately to save useless processing of other plugins.

Edge-cases discussed in various places, like having plugins that need to be called for persisting data, is IMO an invalid use case and bad software design. You don't want a single request to handle multiple application logic steps at once, where one thing relies upon the other.

#5 Updated by Markus Klein 12 months ago

The issue only occurs if the action is a cached action.

#6 Updated by Gerrit Code Review 12 months ago

  • Status changed from Accepted to Under Review

Patch set 2 for branch TYPO3_7-6 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/58580

#7 Updated by Gerrit Code Review 12 months ago

Patch set 1 for branch TYPO3_8-7 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/58581

#8 Updated by Gerrit Code Review 12 months ago

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/58582

#9 Updated by Gerrit Code Review 12 months ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/58582

#10 Updated by Markus Klein 12 months ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#11 Updated by Helmut Hummel 5 months ago

  • Category changed from Extbase + l10n to Extbase

but then the HTML gets cached while response->setStatus() and response->setHeader() do not

What is a use case for caching a plugin action that does a redirect?
Is it for enabling a plugin to perform a permanent redirect for a given URL?
I would be interested in a more concrete use case for something like that.

In any case, the implemented solution seems pretty hacky and results in the complete page never being cached (even worse, cached, but cache deleted in the end),
which is quite a performance hit for such case.

Even worse: even if the plugin action is uncached, the page cache will be deleted anyway, which leads to a unnecessary performance hit.

#12 Updated by Helmut Hummel 5 months ago

  • Related to Bug #88178: Page cache is deleted, when an uncached (Extbase) Plugin performs a redirect added

#13 Updated by Benni Mack 5 months ago

  • Status changed from Resolved to New
  • Target version changed from Candidate for patchlevel to next-patchlevel

Reopened the issue

#14 Updated by Hagen Gebauer 5 months ago

Helmut Hummel wrote:

but then the HTML gets cached while response->setStatus() and response->setHeader() do not

What is a use case for caching a plugin action that does a redirect?

I have an extension with a showAction() that returns a detail view of a database record. I was trying to show a specific “this record could not be found” page with a 404 – instead of the regular 404 page of the website. This is important mostly for records that have been recently deleted but might still be linked somewhere or returned by search engines. I tried to achieve this by forwarding to a specific showNotFoundAction() (see below). Which means the forwarding is being called by an action that has to be cached (the showAction).

I am happy to hear about different solutions for that particular problem. Nevertheless, redirectToUri() is a provided method and the resulting forwarding should in my opinion never be done via an HTML meta tag. And even if it is, there is still the bug of printing the full HTML page into the content part of the template (see snippet in the original post).

    public function showAction(\My\Extension\Domain\Model\MyModel $record = null)
    {
        if (!is_null($record)) {
            $this->view->assign('data', $record);
        } else {
            $uri = $this->uriBuilder
                ->setTargetPageUid($GLOBALS['TSFE']->id)
                ->setArguments(['tx_myextension_plugin'=>['action'=>'showNotFound']])
                ->buildFrontendUri();
            $this->redirectToUri($uri, 0, 404);
        }
    }

#15 Updated by Benni Mack 5 months ago

  • Target version changed from next-patchlevel to Candidate for patchlevel

Also available in: Atom PDF