[HttpKernel] renamed sub-request classes

[kriswallsmith]

The name "HTTP content renderer" was too vague for me, so I renamed things to jive with the concept of sub-requests, which is more specific. I've also renamed the "default" strategy to "inline," which is more descriptive (see #6829).

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	~
License	MIT
Doc PR	~

about trusted proxies

The hinclude strategy should respond to any client IP but the esi strategy should only respond to configured IP addresses. This suggests checking IP should happen downstream of the listener…

about config.yml

I would also like to rework the esi and router_proxy sections of config.yml info one sub_request section that looks something like this…

framework:
    # ...
    sub_request:
        listener:
            enabled: true
            prefix: /_proxy
        esi:
            enabled: true
            trusted_ips: "%kernel.trusted_proxies%"
        hinclude:
            enabled: true

I've changed path to prefix here so the URI could explicitly tell the listener which strategy to call up, allowing that strategy to check the IP address if necessary. For example, /_proxy/esi?… would be caught by the listener, which would then look for an esi strategy, which would check the client IP. A similar request for /_proxy/hinclude?… would call up the hinclude strategy, which would not check the client IP.

[vicb]

esi under sub_request does not seem right to me.

👍 for "inline"

[kriswallsmith]

@vicb Can you say why? We've created a little sub-framework for how sub-requests are rendered and ESI is one of many possible strategies.

[vicb]

esi requests are master requests

[kriswallsmith]

That's an interesting point. If an ESI request is handled by HttpCache it is flagged as a sub-request. However if the request comes from an external proxy it appears to be handled as a master request.

I'm curious to hear what @fabpot thinks of this discrepancy?

[vicb]

Hinc are also master. We should come with something better than subrequest imo.

[kriswallsmith]

I lean the other way… those ESI and hinclude requests should be handled as sub-requests. Not sure how that would work though.

[vicb]

I think per strategy config is covered by an issue from Fabien with a broader scope

[vicb]

those ESI and hinclude requests should be handled as sub-requests

How ? IMO this is not possible by design !

[vicb]

We should come with something better than subrequest imo.

Here I mean the name of the key, ie sub_request

[kriswallsmith]

One possibility…

$response = $kernel->handle($request, $kernel->detectRequestType($request));

…where ->detectRequestType() checks for the path prefix and returns the appropriate flag.

[kriswallsmith]

Or change the ->handle() signature to…

public function handle(Request $request, $type = null, $catch = true);

…and call ->detectRequestType() when null === $type.

[vicb]

but why ? that would a sub-request w/o master and the code would break in some places I think.

[kriswallsmith]

Because a sub-request should always be marked as a sub-request. Can you be more specific about what would break? It's my understanding that sub-requests have no knowledge of their parent request, whether it exists in the same process or not.

[vicb]

But Kris it's the browser or the rp that makes an other master request (on misses) it not a sub request.

[kriswallsmith]

You are correct if your definition of a sub-request is that it must occur in the same process as a master request. I do not agree with this definition, however. My concern is that a {{ render() }} tag will be flagged as a sub-request in development but be handled as a master request in production and the unexpected behaviors this may cause.

[vicb]

To me a master request "is" an HTTP request while a sub-request is something internal to Sf (which implies the same process). I admit I fail to see what a sub-request as an HTTP request would be and why it would be useful ? Is it only because esi requests are sub-requests when HttpCache is used ? (which is not exactly what you said in the previous comment)

[kriswallsmith]

Closing in favor of #6925