[Meta] Allow StreamWrapper's to provide cacheability metadata

Adding a reference to the Flysystem → module which would also greatly benefit from this addition.

Linking 📌 [meta] Modernize File/StreamWrapper API Active which considered using the Flysystem library as base for a modernised Drupal filesystem.

Linking
🐛 [PP-1] S3 Access Denied Request has expired - getExternalUrl() cached beyond presign timeout Postponed and ✨ Add support for private uploads / presigned URLs Postponed as real-world issues where this has been encountered.

🌱 [meta] Use FileUrlGenerator::generate() everywhere, then deprecate generateString() and generateAbsoluteString() Active suggests we should just use URL objects everywhere instead of strings, its a larger break though maybe its better if URL objects are extended to carry capability data themselves and we have streamWrappers return URL objects? Could possibly be done with a method_exists call to a new getExternalUrlObject() method inside the FileUrlGenerator making it BC compatible with old streamWrappers.

We already have a generatedUrl class. But yes, we really should return Url objects so we don't have to switch back and forth

I wasn't aware of the GeneratedUrl class, that's basically my CacheableString proposal :D So that issue would indeed solve this one.

If we implemented the __toString magic method on GeneratedUrl then we could walk the same backwards compatibility path as outlined in the IS (with the same caveats).

I think GeneratedUrl deliberately doesn't support __toString(). They way it works is that Url::toString() has an optional argument where you pass TRUE you get a GeneratedUrl object back.

But as I said, we should instead find a way to transition to return Url objects, so we don't need to do the kind of hacks we currently need to in \Drupal\Core\File\FileUrlGenerator::generate() and can deprecate generateString() and generateAbsoluteString() in the long-term.

Will require some optional interface shenanigans with a new method and then deprecate the old method and deprecate on stream wrappers not yet implementing that. It's unfortunately already called getExternalUrl(). I guess the good thing is that it's very, very rarely called directly. there are no calls to it in core outside of FileUrlGenerator() as far as I see and only 2 in the contrib projects that I have in this project. So we could do a double deprecation and then in D12 go back to the old name if we care enough, or we just live with something like getExternalUrlObject() for the future.

I'll note that just switching to Url objects may not actually solve the lower issue of cacheable metadata on its own.

I believe unless the scheme is 'base' (and maybe entity,internal, or route) Url::class will tag the path as External and locations in core will assume this means no cacheable metadata exists.

For example in in LinkGenerator.php:

    // External URLs can not have cacheable metadata.
    if ($url->isExternal()) {
      $generated_link = new GeneratedLink();
      $attributes['href'] = $url->toString(FALSE); // <-- Request to generate the string without metadata
      return $this->doGenerate($generated_link, $attributes, $variables);
    }

I wonder if the more correct solution should be to remove the assumption that External URL's do not have cacheable metadata and allow the UnroutedUrlAssembler to process outbound route processors to add the data. This would work without changing the streamWrapper getExternalUrl() or modifying the Url class to support cacheable metadata.

Updating title, tags and version number based on recent announcement at https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... →

FWIW, I don't think this is actually a D11 issue, instead, this will need to be done in a backwards compatible way in a minor release.

Yay! Keeping the branch then but removing the title prefix and tag :)

I'm trying to figure out how to rewrite the issue summary for the new direction proposed by Berdir and cmlara. However one thing I'm struggling with is that from what I can see, the metadata is not actually stored on the Url object.

From what I can tell the caching metadata is instead provided by Url processors. So after moving to Url objects in the stream wrapper and removing the assumption that external URLs don't have metadata we would need to either:

1. Apply existing path processors to external URLs too (which might confuse them and break sites)
2. Create a parallel path processing system for external URLs
3. Do something I'm missing

I'm somewhat surprised that Url classes can't have metadata themselves, since in the example cases the cacheability info would already by known when the StreamWrapper creates the Url class (e.g. for time limited URLs) but it might be more difficult to figure that information out later in a path processor.

What are your thoughts here?

The Url object stores information like "the route name is entity.node.canonical and route parameter node is 123". Now, when you generate the actual path from that, the result does need caching information on it, the path can change when the node changes. So what we need here is, to speak JavaScript is string.prototype.getCacheMetadata but since this is PHP we have a GeneratedUrl class which has a getGeneratedUrl() method to get the generated path string and then the three cache metadata methods. Not elegant but what can one do? The day may come when PHP becomes a Smalltalk derivative but it hasn't yet.

Currently StreamWrapperInterface::getExternalUrl() returns a string, what Berdir suggests wisely is a StreamWrapperInterface::getExternalUrlObject() method which instead return GeneratedUrl objects which would allow collecting caching metadata. Right now in core only the FileUrlGenerator calls getExternalUrl() so the impact would be small. But, once you have a GeneratedUrl object which is an instance of BubbleabbleMetadata you can do real neat tricks with it like make a CacheableResponseInterface out of it and so forth.

So the proposal is this: add getExternalUrlObject() to StreamWrapperInterface returning GeneratedUrl.

Now comes the version dance... Since this would break existing stream wrappers, I think it would need to be 11.x. We could deprecate getExternalUrl() in 11.x and in 12.x rename getExternalUrlObject () to getExternalUrl() because by then there should be no calls to the old methods. Or we can just live with getExternalUrlObject(). We could even do some interface shenanigans where we introduce an ExternalUrlObjectCapableStreamWrapperInterface , deprecate, remove etc etc etc my head hurts :D

I would agree, CHX has summed up the solution well. The best long term solution is having the streamWrapper return a GeneratedUrl object. This is where we are generating the URL so this is where it makes the most sense to catalog the related dependencies.

I would suggest the method could be getGeneratedUrl() if we want something that actually describes what it does (I'm not fond of getExternalUrl it name misleads what it does) to avoid a version dance around renaming. No need to make this more complicated than it needs to be, lets make this a simple Deprecate a method while adding a replacement method routine (along with the associated new interface for new methods/interface deprecation routine)

After that than we can solve the problems of getting that metadata to where it needs to go in a followup issue.

> Currently StreamWrapperInterface::getExternalUrl() returns a string, what Berdir suggests wisely is a StreamWrapperInterface::getExternalUrlObject() method which instead return GeneratedUrl objects which would allow collecting caching metadata.

That is not what I suggested. GeneratedUrl doesn't help to solve 🌱 [meta] Use FileUrlGenerator::generate() everywhere, then deprecate generateString() and generateAbsoluteString() Active . I suggested a URL object, but wasn't aware that it doesn't support cacheability metadata. That is unfortunate. We either need a version of Url that does that or we need a second by-reference argument to collect cacheability metadata, like we do elsewhere.

Thanks for the additional insights.

Should we create a separate issue to discuss adding cacheability metadata to the Url class? #3 already suggested it and #14 also indicates it would help.

I have a week of holiday starting, after that I'd be happy to dive into this during Drupal Dev Days and see if I can work on a fix. However, there seem to be some other moving parts, like the references issues, that I'll have to familiarize myself with before then. Unless anyone beats me to it, I'll also use that time to provide an updated issue summary, since the current proposed solution is based on incorrect assumptions.

I created ✨ Allow links to external routes to gather metadata in LinkGenerator::generate Needs work to solve #7 which seemed like an easier step to start with.

I'm converting this into a meta issue since I think there's no single patch that can solve this issue, but instead we can fix this as various sub-issues that together solve the problem.