Contrib.social

file_create_url() causes LogicException when used with certain stream wrappers

Open on Drupal.org →
Created on 6 April 2017, over 7 years ago
Updated 18 February 2023, over 1 year ago

While working on a Rest-resource we have an issue with rendering Url's to private files.

We used $file->url() and that resulted in this error:

The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early.

Diving into it looks like PrivateStream::getExternalUrl() and UrlGeneratorTrait::url() are here to blame, because these functions do not pass the collect_bubbleable_metadata attribute when rendering the url.

We used:

$file->url()

This will call file_create_url() located in /core/includes/file.inc

At some point in file_create_url() the appropriate wrapper is loaded and the method getExternalUrl() will be called.

// Attempt to return an external URL using the appropriate wrapper.
if ($wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($uri)) {
  return $wrapper->getExternalUrl();
}
else {
  return FALSE;
}

The function getExternalUrl on the PrivateStreamWrapper calls
url() in Drupal/Core/Routing/UrlGeneratorTrait.php
This doc-comment says that \Drupal\Core\Routing\UrlGeneratorTrait::url() follows \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute(), but it does not.
The collect_bubbleable_metadata attribute is missing.

🐛 Bug report
Status

Needs work

Version

10.1 ✨

Component
File system  →

Last updated about 22 hours ago

Created by

🇳🇱Netherlands ndf Amsterdam

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇺🇸United States smustgrave

    This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

    Seems at some point test coverage was lost so adding back the tests tag.

    This could use an issue summary update to show what the proposed solution, remaining tasks, api changes, etc.

    New functions should be typehinted for 10

    Adding a new interface will require a change record to announce to others.

    Thanks.

  • Comment over 1 year ago →
    🇫🇷France andypost 
    +++ b/core/lib/Drupal/Core/StreamWrapper/PrivateStream.php
@@ -45,8 +45,25 @@ public function getDirectoryPath() {
+   * @return \Drupal\Core\Url
+   *   A Url representation of the resource.
...
+  protected function getUrl() {
     $path = str_replace('\\', '/', $this->getTarget());
-    return Url::fromRoute('system.private_file_download', ['filepath' => $path], ['absolute' => TRUE, 'path_processing' => FALSE])->toString();
+    return Url::fromRoute('system.private_file_download', ['filepath' => $path], ['absolute' => TRUE, 'path_processing' => FALSE]);

    Sounds like exactly this method should be public and its consumers will use it as string and can manage cache metadata

  • Comment over 1 year ago →
    🇫🇷France andypost
  • Comment over 1 year ago →
    🇫🇷France andypost
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024