[Templating] Handle uri query strings passed to templating extension and helper

[rpkamp]

Q	A
Branch?	1.0
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#903
License	MIT

Gracefully handle any query strings in paths passed to the templating filters.
See added tests for behaviour.

[rpkamp]

I'd love to hear a better name than $pathParts 🙄

I tried list($path, $queryString), which looks a lot better, but doesn't work when the original path doesn't contain a question mark (undefined index 1)

[felipyamorim]

you can try this, to keep list function use.

list($path, $queryString) = array_pad(explode('?', $path, 2), 2, null);

[rpkamp]

It works, but is quite hard to read IMO.

[felipyamorim]

I agree, $pathParts sounds betters.

Waiting this merge.

[robfrawley]

We've discussed this in the past and one other idea brought up was to expose a configuration option that would enable this behavior. Also, isn't it possible that such handling should exist outside the templating component as well?

[rpkamp]

I consciously restricted the behaviour to the templating component because of the nature of templates, where it is easy to mix up paths and URLs. Other components should in my opinion only deal with paths and not concern themselves with query strings.

In terms of hexagonal architecture I think of the templating component as an infrastructure thing where user input is received that might be messy, whereas after that we enter the application layer, where input should be normalised to adhere to the application spec. So that is why I've made the templating component forgiving of query strings and fix them when sending them to the application and adding them back to the application response, but the rest of the bundle is still blissfully unaware of query strings.

[felipyamorim]

+1

[robfrawley]

So, while I prefer this behavior over our current handling, I consider this a BC break. Many filesystems are happy to allow a filename such as "my-cool-image?with=an&odd=name.jpg" and this change would break such a usage.

As such, we need to default to the old behavior and add the ability to enable this behavior via a configuration toggle for the 1.x branch. This configuration option can then have its default changed on the 2.x branch to enable this handling by default.

I already have a commit that adds the configuration required to do this, and I'll push it to this PR shortly, as I'd like to include this in the next release. Just wanted to give everyone a heads up as to any progress.

[rpkamp]

This PR has been open for a long time (since May of this year).
Anything I can do to advance the merging of this PR?

[makasim]

As such, we need to default to the old behavior and add the ability to enable this behavior via a configuration toggle for the 1.x branch. This configuration option can then have its default changed on the 2.x branch to enable this handling by default.

@rpkamp you could as suggested by @robfrawley or simply change the base branch to 2.0

[rpkamp]

I've been thinking about this some more and have come to the conclusion that this PR solves the wrong problem. The problem is that the filter is applied to the output of asset, which is very inefficient as it would first copy the image from where-ever it is to the web root and then modify the copied image in the webroot.
So instead of allowing query strings in the render extensions we should discourage people from loading images using asset, but set up a correct loader for the path where there images are instead.

As such, I'm closing this PR.