FluidTYPO3 / vhs

TYPO3 extension VHS: Fluid ViewHelpers
https://fluidtypo3.org
Other
190 stars 229 forks source link

BUG: Wrong order of middleware registration, HTML gets cut off when including assets #1893

Closed tastendruecker closed 5 months ago

tastendruecker commented 6 months ago

I have checked that the bug exists in the dev-development branch Yes

I have checked that there are no already open issues or recently closed issues about this bug Yes

Describe the bug Asset inclusion should/must be executed before content-length calculation done by the core's class cms-frontend/Classes/Middleware/ContentLengthResponseHeader.php. Because middlewares are executed "last in, first out", \FluidTYPO3\Vhs\Middleware\AssetInclusion::class should be registered to be executed AFTER 'typo3/cms-frontend/content-length-headers'. That way it gets executed before the core's middleware. That also means, fiddling with header "Content-Length" inside of AssetInclusion is no longer necessary.

Currently, AssetInclusion is executed after content-length calculation. That means incorrect length headers get sent to clients and many of them ignore the contents that exceed the content-length.

Unfortunately, documentation provided by TYPO3 does not provide any info (AFAIK), but you may have a look at https://reelworx.at/blog/detail/correct-manipulation-of-generated-html-in-typo3/

To Reproduce Steps to reproduce the behavior:

  1. Include assets (JavaScript, CSS...) with the corresponding viewhelper inside a Fluid template
  2. Configure TYPO3 to send content-length header, i.e. set TypoScript option config.enableContentLengthHeader to true
  3. Request a cached (!) version of the page without being logged into the back-end.
  4. HTML most likely will be cut off at the end. Happens at least in Firefox, Chrome, and Edge

Thanks in advance, Sven

NamelessCoder commented 6 months ago

\FluidTYPO3\Vhs\Middleware\AssetInclusion::class should be registered to be executed AFTER 'typo3/cms-frontend/content-length-headers'

That sounds quite wrong to me. I'm not sure that's actually the case - if that were so, then TYPO3 would be out of the box configured to do output compression on content, before the content is created; and do site-resolving after the page has been resolved. As far as I am aware, TYPO3 will process every middleware in their defined order (it will not switch order of the last "chunk" of middlewares after a certain point). So when you register a middleware to come before something else, then that middleware will be processed before the relative middleware.

It is however possible that config.enableContentLengthHeader somehow changes when the content length calculation happens so it no longer happens in the content-length-headers middleware, that it calculates length on an older version of the output string, or that VHS's calcualted content length is somehow overwritten.

tastendruecker commented 6 months ago

Hi @NamelessCoder,

it's quite puzzling. The last few days I had some troubles to get used to the way, TYPO handles the order inside the middleware stack. Within the registration, "before" actually means, that the referenced middlewares must be executed before the currently registered middleware. And "after" means, the referenced middlewares must be executed after it.

It's a bit cryptically worded, but one may read it here: https://docs.typo3.org/m/typo3/reference-coreapi/main/en-us/ApiOverview/RequestLifeCycle/Middlewares.html#configuring-middlewares

In my current project I found that using this registration of the VHS middlewares does the trick:

return [
    'frontend' => [
        'fluidtypo3/vhs/asset-inclusion' => [
            'target' => \FluidTYPO3\Vhs\Middleware\AssetInclusion::class,
//            'before' => [
//                'fluidtypo3/vhs/request-availability',
//            ],
            'after' => [
                'typo3/cms-frontend/content-length-headers',
                'typo3/cms-frontend/output-compression',
            ],
        ],
        'fluidtypo3/vhs/request-availability' => [
            'target' => \FluidTYPO3\Vhs\Middleware\RequestAvailability::class,
            'before' => [
                'typo3/cms-core/normalized-params-attribute',
            ],
        ],
    ],
];

With this config, content-length headers get produced after the assets have been inserted. Works like a charm... And: No need for VHS extension to do the content-length calculation on its own or to disable those headers.

NamelessCoder commented 6 months ago

@tastendruecker Would you mind going to the "Configuration" module in your TYPO3 install and selecting the "PSR-15 Middlewares" category, expand the "frontend" section and grab a screenshot for me? I'm really curious how that configuration makes the middlewares array look. On my system(s) the VHS asset middleware sits before the content-length and output-compression middlewares and when those are iterated, that would (afaics) cause the processing to happen in the right order. If those are sorted differently on your system and it causes the right behavior then I'm really at a loss as to what's going on.

Before: List of middleware identifiers. The middleware itself is executed before any other middleware within this array.

This means the exact opposite of "the middlewares in this array must be executed before the current middleware".

From what I read in the documentation: if I configure content-length and output-compression in the "before" array, then that means the VHS asset inclusion must happen before those two. In human words the current configuration reads as and means: "VHS asset inclusion must happen before content length is calculated and before output compression happens" - and that is 100% correct.

There just has to be some kind of side effect which coincidentally is making this work on your system with an intentionally wrong configuration.

tastendruecker commented 6 months ago

Hi @NamelessCoder,

sure, this is the middleware stack of my project: Configuration-·-HTTP-Middleware-PSR-15-My-Site-TYPO3-CMS-12-4-9-

If you take a tool like xdebug and set breakpoints at the return statements of the registered middlewares' process() methods, you may see the order in which they are executed: First one is TYPO3\CMS\Core\Middleware\ResponsePropagation, last one is TYPO3\CMS\Frontend\Middleware\TimeTrackerInitialization, at least in my project ;-)

Additionally, the PHPDoc comments of methods add() and lazy()of TYPO3\CMS\Core\Http\MiddlewareDispatcher state that the stack gets executed "last in, first out".

I suppose, this is no kind of side effect, but the documentation is misleading. Just my 2 cents...

NamelessCoder commented 5 months ago

The confusion stems from the fact that middlewares are executed in the order they are configured, so the before and after instructions do work the way the configuration states. But most middlewares work by calling $handler->handle($request); as the first line. Middlewares therefore have two methods of operation:

(TimeTrackerInitialization does both: sets the timer, then executes the next - and next, and next, etc. - middlewares and reads the timing result after, before finally returning - and VHS's other middleware executes things before the next neighbor).

When they execute things after the next neighbor that gives the effect that the stack is processed "in reverse" (it isn't really, though): the middleware that comes before another, executes the middleware that comes after. This is to say that it's actually not an effect of the stack itself - it's an effect of how each item in the stack operates.

But it is true that this means the first thing VHS does in the asset inclusion middleware is execute the content-length middleware which is indeed the wrong order. What we want instead is for the content-length middleware to execute VHS asset inclusion (or another middleware that then executes VHS asset inclusion) first and then assign its content-length header. But this is not because the stack is executed in reverse - it's because the content-length middleware executes the next neighbor before doing it's own operation.