isLocal() uses $this->uri, but that's a FieldItemList object, it should use $this->getFileUri()

I can possibly also be simplified a lot by using \Drupal\Core\StreamWrapper\StreamWrapperManager::getViaUri() and then getType().

CommentFileSizeAuthor
#4 file_entity-2846057-4.patch646 bytespifagor
#2 file_entity-2846057-2.patch7.79 KBpifagor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor commented
Status: Active » Needs review
FileSize
file_entity-2846057-2.patch7.79 KB

Patch. Was unable to verify, test pliz

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work

Thanks.

+++ b/src/Form/FileAddArchiveForm.php
@@ -89,7 +89,7 @@ class FileAddArchiveForm extends FormBase {
             $file = File::create([
-              'uri' => $file->uri,
+              'uri' => $file->getFileUri(),
               'filename' => $file->filename,
               'status' => FILE_STATUS_PERMANENT,

this is different, $file here is the return value of file_scan_directory, pretty sure those are stdClass objects and shouldn't be change.

Same for all other references except the one in isLocal(). You really only need to change that one.

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor commented
Status: Needs work » Needs review
FileSize
file_entity-2846057-4.patch646 bytes

new patch

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Thanks, this is what I did in our project as a replacement of isLocal(), which is think is much easier to read:

$stream_wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($file->getFileUri());
if ($stream_wrapper && $stream_wrapper->getType() & StreamWrapperInterface::LOCAL) {

Want to try and replace the current implementation with something like that? We could put \Drupal::service('stream_wrapper_manager') in a separate method so we can document the return value and get proper method autocomplete.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work
dsdeiz’s picture

dsdeiz CreditAttribution: dsdeiz commented

Curious if other methods need to be changed as well. I see 3 methods using the same logic:

  • isReadable()
  • isWritable()
  • isLocal()

isLocal() seems to be the only one not using $this->getFileUri(). Should others be changed as well or just specifically isLocal()? Also thought about accessing the stream_wrapper_manager service using dependency injection although I see a lot of other services accessed from \Drupal::service().