Imported PO files have no public URL but have a link at admin/content/files

I've just got this one with beta14.

The reason is only that imported po files -that as said above are treated as regular files- cannot get a public URL -by design-.

Wondering whether we should just filter them out in the view, or soften the error, maybe returning an empty string instead of an Exception, which looks like too much to me.

I can confirm that this continues to exist in release 8.1.1.

I stumbled across this after importing translations. Could not rebuild xmlsitemap nor even access admin/content/files page.

Created patch that replaces exception with logger warning.

Rerolled the patch.

Probably linked to https://www.drupal.org/node/2821123
I get this on drupal 8.3.2 too.

Reviewed on Drupal 8.3.2 and 8.2.4 with applied patch - no error while importing po-files. Can be merged.

I get the fatal as soon as I visit /admin/content/files after having uploaded a PO file. I.e. without clicking on any particular file (I never reach the actual list). That seems at least major to me. I guess not critical because /admin/content/files is a view, so you can reconfigure to not link the files (whether it's reasonable to expect people to do that, though, is another question...)

Here is an alternate patch, that makes file_create_url() a bit safer, by checking the stream wrapper, whether it supports creating web-accessible URLs. As far as I can tell, StreamWrapperInterface::VISIBLE exists for that notion, but not entirely sure. This worked fine for me, though. An uploaded PO file was not linked, while a normal image upload had a regular link.

Oops, forgot the patch.

Hmm... so the temporary stream is declared hidden, but it explicitly provide a web-accessible URL. So either my assumption on the VISIBLE flag is incorrect, or the temporary stream is declaring the wrong flag. Here's a patch for the latter, which should be green. I'm not sure whether making the temporary stream visible would have any security implications. I noticed the private stream is visible as well, so I think we are good, but not sure to be honest.

A workaround until this is fixed (/or you can't or don't want to patch this module) is to add a filter criteria to the file view (admin/structure/views/view/files):

File: File MIME type (!= application/octet-stream)

This wil filter out all the files with MIME type "application/octet-stream".
In my case this only applies to the .po file('s), and as far as i can tell (/find), this will also be applied to files without an extension.

+++ b/core/includes/file.inc
@@ -230,7 +231,9 @@ function file_create_url($uri) {
-    if ($wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($uri)) {
+    /** @var \Drupal\Core\StreamWrapper\StreamWrapperInterface|false $wrapper */
+    $wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($uri);
+    if ($wrapper && ($wrapper->getType() & StreamWrapperInterface::VISIBLE)) {

I don't see why we'd want to do this — why would we only allow file URLs for "visible" file URIs, which is defined as Exposed in the UI and potentially web accessible.

It's perfectly valid to generate a file URL for a file URI that is not exposed in the UI, and not web-accessible?

Looking at it some more, I think I was wrong.

const READ = 0x0004;
const WRITE = 0x0008;
…
const HIDDEN = 0x000C;

HIDDEN = READ + WRITE

Which means that VISIBLE indeed kinda means "can get a URL for it".

But I think that translations:// is supposed to be (and remain) HIDDEN though. The whole point is that it's a file that doesn't surface anywhere. I think the bug is simply that the files listing is not respecting the visibility implications of the stream. The files listing should omit those files altogether, they're not meant to be exposed anywhere.

IOW: there are two bugs here:

1. file_create_url() trying to create file URLs for every file URI
2. the views.view.files View config entity

Solving #1 is what #20 does (but there is a certain BC risk involved in this). But it still requires #2 to be solved. Solving #2 means we don't really need to solve #1.

Wim, I ran into this issue tonight, on Drupal 8.4.x.

Should admins even be able to see Temporary files? If not, the view filter can either ignore those or some higher up in the call stack could ignore Temporary files when "rendering" pages that interact with Temporary files. Just a thought.

Just a quick follow up. You probably know this already, but my screenshot is a list of files in the files_managed table. If I remove those records and clear cache, the error goes away...

Preventing the temporary files from showing is one thing. But, I guess a more important question, is why are we storing temporary files in the file_managed table? I notice there is a queue table, which seems to be used for importing .po files in. If that is the case, do we really need / want to dump temporary files into the file_managed table? I'll admit, I didn't dig too deep in the code to review the code that manages temporary files.

Should admins even be able to see Temporary files?

I think they should not!

If not, the view filter can either ignore those or some higher up in the call stack could ignore Temporary files when "rendering" pages that interact with Temporary files. Just a thought.

Yes, a View Filter is what we need I think.

But, I guess a more important question, is why are we storing temporary files in the file_managed table?

Good question. I don't know the full history there. I can think of some potential reasons, but I don't know for sure.

Here is a patch file to fix it at the view level. I dug through the file module code a bit, it seems that temporary files are stored in the file_managed table, and are supposed to be cleared out (via cron?). None-the-less, the Content Files won't show them any longer with this patch.

The files listing should omit those files altogether, they're not meant to be exposed anywhere.

I don't see how it's possible to guarantee that with Views. We can change the default views, but then people can go in and change the View back and then they will still get a fatal error, no?

In other words, I think this:

Solving #2 means we don't really need to solve #1.

is actually incorrect.

So if we don't want to touch file_create_url() we need to look at why the translation files end up in {managed_file}. This is more specific than just asking

But, I guess a more important question, is why are we storing temporary files in the file_managed table?

though, as temporary files are used for other things, as well, most importantly for normal file fields before the host entity has been saved. For this issue we need to look at the translation files in particular.

We actually have a file_unmanaged_* API, so we have the capability of interacting with files that are not in {file_managed}. So instead of changing something in Views, the other solution would be to change the interface translation form to no longer use managed files. Looking at the code a bit it seems that may actually be possible. \Drupal\locale\Form\ImportForm actually use a form element of type file (instead of managed_file), but it then calls file_save_upload() in ::validateForm(). Later in ::submitForm(), where the batch to import the translations is set up, the file is actually re-converted to a \stdClass object, so the consuming API actually doesn't need this to be a managed file. So basically we "just" need to avoid that file_save_upload() call. Not sure how annoying that will be, though, as that function does quite a lot of stuff.

@tstoeckler I agree with you about not changing the view. The problem needs to be resolved high up the call stack.

Thanks for explaining the ImportForm details. I believe the one reason they are stuffed in file_managed table is to utilize the db when installing new modules. When a new module is installed the progress bar seems to be illustrating a lot of translation work is being done...

It might be worth while to see what is happening after the .po file is stuff in the file_managed table. Maybe there is a second process that runs during the module install, to import the .po into the locale / translation layer of remaining tables. Just a guess.

If this is the above is somewhat accurate, maybe the cleanup process is the culprit. Originally, I was led to this issue due to a failure while installing a module. So if a failure happens, the cleanup process is not called nor is there a way to call it again (I did run cron a few times and .po files stayed in the file_managed table).

Thoughts?

Any updates?

I am still getting issues when I try to upload a po file.

#20 works for me, at least, I can import the PO files without any issues. Its throwing error otherwise.

I'm very confused. The .po file is only used once after uploading, why not store in /tmp directory? I tried to run rm -rf sites/default/files/translations/ and drush cr. The translation content still work. Should this not be stored in the correct directory /tmp when the translation file is uploaded?

Why can't we simply not throw an exception in an interface's method that does not describe this?

Patch #38 works.

I think to match the behaviour and expectations of file_create_url()'s return value we should return FALSE because implementations that call file_create_url() already expect a FALSE here.

The docs for file_create_url() are

 * @return string
 *   A string containing a URL that may be used to access the file.
 *   If the provided string already contains a preceding 'http', 'https', or
 *   '/', nothing is done and the same string is returned. If a stream wrapper
 *   could not be found to generate an external URL, then FALSE is returned.

We should update these docs too - to note that FALSE is also returned if the file has no external URL.

Also I think we should have a test for this major bug fix.

Here is the patch with the test that should expose the problem and another one that helps to overcome it.

1. 2449895-42-do-not-commit.patch - must fail.
2. 2449895-42.patch - must pass.

Patch looks good to me. I still think #20 is valid, as well, but this makes sense in its own right. Will let @alexpott RTBC this, though.

Oops, version change was unintentional.

Patch #38 works.
Thank you very much

Same problem with Drupal 8.6.4 and patch #42 fixes the problem, It seems RTBC.

Patch applied and works with the latest version.

+++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperInterface.php
@@ -145,8 +145,9 @@ public function getUri();
-   * @return string
-   *   Returns a string containing a web accessible URL for the resource.
+   * @return string|false
+   *   Returns a string containing a web accessible URL for the resource or
+   *   FALSE if a stream doesn't/cannot provide the URL.
    */
   public function getExternalUrl();

We need a CR to announce the API widening here.

Another way we could do this fix with less change is to do a try catch in file_create_url() which doesn't seem a bad idea. Less change. Something could already expect the exception and not expect a FALSE.

Please forgive my confusion, i cannot understand if it is the same issue with what i am dealing with. I use 8.7.5 latest stable version and when i am trying to import a .po file, i get the same error "LogicException: PO files URL should not be public", the file is transferred in translation folder but because of the php exception the site is corrupted.
The difference with this issue is that i don't see this file in content/files at all.

Hi,
Reviewed on Drupal 8.7.5 with applied patch - no error while importing po-files.

Needs work to implement #50

Yes we can apply exception handling (try/catch) in "file.inc" file. Please see this patch.

Thanks for the patch @supriya1992

A few points:

1. +++ b/core/includes/file.inc
   @@ -215,53 +215,59 @@ function file_stream_wrapper_uri_normalize($uri) {
   +  try{
   

   I believe there should be a space after try before {.
   In general, it's important to use PhpcodeSniffer to check you're following Drupal coding standards.

2. +++ b/core/includes/file.inc
   @@ -215,53 +215,59 @@ function file_stream_wrapper_uri_normalize($uri) {
   +    \Drupal::logger('some_execution_error')->warning('<pre>' . print_r($e, TRUE) . '</pre>');
   +    drupal_set_message('There is some problem in execution', 'error');
   
   

   The code inside catch is not right - the messages are not real.

   More fundamentally, I'm not sure we want to display or log any error. Possibly failing silently is what we want here.

3. +        return $uri;
          }
   -
   -      // Append fragment.
   -      if ($options['fragment']) {
   -        $path .= '#' . $options['fragment'];
   +      else {
   

   Your patch is very hard to understand, because even though it only makes a very simple change, the hunks are all broken up in ways that seem unnecessary. This may be caused by something you've done, or it may be caused by the tools you're using to create the patch. I don;t have any advice on how to fix, but it makes the patch extremely hard to review

4. Previous patches in this issue have tests that we need, they should be added to this patch.
5. Please provide an interdiff when you post patches that build on previous patches.
6. Assigning an issue to yourself means "Don't work on this, I'm working on it right now" - I'm not sure this is what you meant.

Patch #42 works for me on Drupal 8.6

Reroll the patch for 8.9.x.

I also replace the usage of drupal_set_message('There is some problem in execution', 'error'); for \Drupal::messenger()->addError(t('There is some problem in execution'));

Oupsy ... I didn't see the message on #58 and reroll the patch #56 without noticing it miss many changes of previous patches ... I will work on another patch (#63) to fix my mistake.

Based on the message on #58, here is a patch where I try to re-apply all the changes from #42 and the try-catch of #56.

I made 2 interdiff to show my changes against those two patches.

I hope I made no mistake or regression ^^. Let's test it.

1. +++ b/core/includes/file.inc
   @@ -213,53 +214,57 @@ function file_stream_wrapper_uri_normalize($uri) {
   +          $path .= '?' . UrlHelper::buildQuery($options['query']);
   ...
   +  catch (\Exception $e) {
   +  }
   

   I recommend to return FALSE when a exception is raised.

2. +++ b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php
   @@ -368,6 +368,41 @@ public function testCreatedLanguageTranslation() {
   + public function testPoFileImportAndAccessibilityOfFilesOverviewViewsPage() {
   

   The coding styles this method look wrong (indentations).

Apply the changes from #64.2 about the indentation, sorry for that ...

I also apply the suggestion from #64.1, but instead of returning FALSE in the catch, I prefer here returning FALSE at the function bottom and removing unnecessary previous return FALSE in a nested else/if condition upper.

In case anyone needs this for core 8.7 (as I do): #42 works with 8.7.9

Patch in #42 works great with 8.7.10 too!

I simplified the solution as per suggestion by @alexpott in #50: no API changes needed here and thus no CR need either, as the problem can be solved by one try-catch.

Also, the try-catch block was unnecessarily large so I simplified that too.

The test seemed to be fine to me so I didn't touch that.

Oops, fixed the paths from my previous patch. Sorry.

hen this issue is fixed, the project maint

1. We should document the \LogicException on the interface
2. We can improve the logic slightly in file_create_url and we should catch the specific exception - not all the exceptions.

3. +++ b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php
   @@ -373,6 +373,41 @@ public function testCreatedLanguageTranslation() {
   +    // The problem this test cover is exposed in an exception that is thrown
   +    // by the "\Drupal\locale\StreamWrapper\TranslationsStream" when "views"
   +    // module provides a page of files overview. Refer to the issue to find
   +    // more information.
   

   The comment "refer to the issue to find more information" feels unnecessary - there's git blame for that.

4. +++ b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php
   @@ -382,12 +417,12 @@ public function testCreatedLanguageTranslation() {
      public function importPoFile($contents, array $options = []) {
   -    $file_system = \Drupal::service('file_system');
   -    $name = $file_system->tempnam('temporary://', "po_") . '.po';
   -    file_put_contents($name, $contents);
   -    $options['files[file]'] = $name;
   +    $file_system = $this->container->get('file_system');
   +    $file_path = $file_system->tempnam('temporary://', 'po_') . '.po';
   +    file_put_contents($file_path, $contents);
   +    $options['files[file]'] = $file_path;
        $this->drupalPostForm('admin/config/regional/translate/import', $options, t('Import'));
   -    $file_system->unlink($name);
   +    $file_system->unlink($file_path);
      }
   

   These changes are purely cosmetic as far as I can see. In functional tests \Drupal::service() is preferred over $this->container for reasons.

Changing back to RTBC as the failing test (#73) was a transient error and the tests are passing again.

I am on core 8.8.5 and @alexpott patch from #72 applied successfully!

Sorry for the delays on patch reviews here. This is a tricky issue with a complicated history, so it's difficult to review despite being a small patch.

1. +++ b/core/includes/file.inc
   @@ -254,12 +254,14 @@ function file_create_url($uri) {
      else {
        // 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;
   +      try {
   +        return $wrapper->getExternalUrl();
   +      }
   +      catch (\LogicException $e) {
   +      }
        }
      }
   +  return FALSE;
   

   This is a much wider change than just not throwing exceptions for .po files in views. I think it at least needs a change record and a broader test to enforce that this is now the design behavior of the API. However, are we sure we want to make this broad change? file_create_url() is used in way more places than just the manage file listing. Could we catch it up a level?

2. +++ b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php
   @@ -373,6 +373,41 @@ public function testCreatedLanguageTranslation() {
   +   * Tests that imported PO files aren't break the UI provided by "views".
   

   Tests that imported PO files don't break the file listing view.

   (It's not about Views UI and the sentence needs a little grammar work.)

3. +++ b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php
   @@ -373,6 +373,41 @@ public function testCreatedLanguageTranslation() {
   +  public function testPoFileImportAndAccessibilityOfFilesOverviewViewsPage() {
   

   It's not testing accessibility? I guess this means "availability"?

   How about:
   testPoFileImportAndFileListing()

4. +++ b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php
   @@ -373,6 +373,41 @@ public function testCreatedLanguageTranslation() {
   +    // and has an access to the overview of files in a system.
   

   ...and has access to to the files overview listing.

5. +++ b/core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php
   @@ -373,6 +373,41 @@ public function testCreatedLanguageTranslation() {
   +    // The problem this test cover is exposed in an exception that is thrown
   +    // by the "\Drupal\locale\StreamWrapper\TranslationsStream" when "views"
   +    // module provides a page of files overview.
   

   Ensure that an exception is not thrown on the admin file view as a result of the restrictions on .po files.

#72 worked for me too, thanks xD

On core 8.8.6 patch #72 applied successfully. Thanks for that patch ! Kinda strange anyway to list .po file here ?

Re #77.1 file_create_url() already documents this.

 * @return string
 *   A string containing a URL that may be used to access the file.
 *   If the provided string already contains a preceding 'http', 'https', or
 *   '/', nothing is done and the same string is returned. If a stream wrapper
 *   could not be found to generate an external URL, then FALSE is returned.

We're making the code confirm to the documentation. It's a bug that file_create_url() is not returning FALSE here. It's the expected and documented behaviour. I'm not sure that we should have a change record for making something behave as per the documentation.

#77.2 Fixed
#77.3 Fixed
#77.4 Removed the text - the permissions and code are enough documentation
#77.5 Fixed and added an @see to the code that throws the exception.

#80 work for me :)

#80 works for me too

On core 8.9.1 patch #80 applied successfully.

Applied #80 to drupal/core (8.9.15) and also fixes an error when importing translation files at /admin/config/regional/translate/import

Thanks.

Patch does not apply on 9.3.x, but doesn't seem to be needed anymore.

I have the same issue on 9.3.x but the patch doesn't work.

Tried to reroll the patch in #80 for 9.3.x, not sure if this will work. Added a diff file for reference, thanks!

Hi all,

I can confirm patch in #92 fixes the exception in 9.3.12, applies cleanly. Cheers for the patch.

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.

Think the method should be deprecated first right? This could break existing sites.

Still needs a change record.

Added a deprecation notice and created a (draft) change record https://www.drupal.org/node/3378735

For info, patch #92 worked for me.
Config: PHP 8.3.3, Drupal 10.2.3.