backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Trying to access array offset on value of type null in file_view_file() (line 3306 of ../core/modules/file/file.module) #6674

Open alanmels opened 1 month ago

alanmels commented 1 month ago

Description of the bug

The issue might be stemming from PHP version 8.1.2-1 that probably doesn't silently suppress such warnings as previous versions did, but still needs to be addressed.

Steps To Reproduce

To reproduce the behavior:

  1. Add a file field to any content type's comment as shown below allowing various types of extentions:
Screenshot 2024-08-01 at 1 24 55 AM
  1. Switch to Comment Display and choose Rendered file format to display.
  2. Post a comment uploading a few different types of files. In my case they were three files of .xlsx, .numbers and .pdf extensions.
  3. Check the website log to see the warning message:
Warning: Trying to access array offset on value of type null in file_view_file() (line 3306 of /var/www/site/public_html/core/modules/file/file.module).

Additional information

I know some extension files are not renderable, however still for purposes of this particular project we need to leave the display format intact for image and audio files, which are perfectly rendered. Some other files such as, for example, text files should be just displayed as list of files without causing the reported warning.

argiepiano commented 1 month ago

Hi @alanmels. I'm not able to reproduce.

I assume this is a field with unlimited cardinality, correct? (that's not clear in your OP).

I added three files to a comment (xlsx, txt and pdf). I set the display to "Rendered file". The comment, when rendered, shows the links to 3 files and no warning. I'm using PHP 8.1.13.

It may that there is something missing in your description, or that there is some interaction with a contrib module?

EDIT: It may have to do with a File entity view mode that doesn't exist. Are you able to debug the code? Between lines 3299 and 3300 can you add

debug($display, 'Display');
debug($view_mode, 'View mode');

Then check admin/structure/file-types/manage/document/display to see if the view mode actually exists.

alanmels commented 1 month ago

I've just replicated it again on a fresh Backdrop site. Could you please re-create my setup looking at these screenshots and upload this file to a comment:

Screenshot 2024-08-01 at 10 20 31 PM Screenshot 2024-08-01 at 10 20 50 PM Screenshot 2024-08-01 at 10 25 50 PM
alanmels commented 1 month ago

For some reason, I could not upload .numbers file, so had to suffixed it with .txt, so please remove that part after downloading this file:

1719846235-2061.numbers.txt

alanmels commented 1 month ago

Line 3306 looks like:

$formatter = $display['formatter'];

and when you upload a file with extension .numbers (xls equivalent of Apple's Numbers), the $display returns empty. Replacing the line with:

$formatter = $display['formatter'] ?? '';

makes the warning go away. But I am not sure if it's the best solution. Anyway, there should be some kind of fallback method in case if Backdrop doesn't recognize the file extension to apply a formatter.

alanmels commented 1 month ago

Replicated on both 7.4.33 and 8.1.2, so it's not about PHP version.

alanmels commented 1 month ago

Oh, and to see the notice on the same window as my screenshots show, you need to go to /admin/config/development/logging and check on All messages under Error messages to display

alanmels commented 1 month ago

For the sake of easier replication, please ditch all the comment part and get it on the node itself:

Screenshot 2024-08-02 at 11 57 16 AM Screenshot 2024-08-02 at 11 58 08 AM Screenshot 2024-08-02 at 12 00 51 PM Screenshot 2024-08-02 at 12 00 25 PM Screenshot 2024-08-02 at 12 01 23 PM
alanmels commented 1 month ago

After putting that dpm() in line 3299 like so:

function file_view_file($file, $display = 'full', $langcode = NULL) {
  if (!isset($langcode)) {
    $langcode = $GLOBALS['language_content']->langcode;
  }

  // Prepare incoming display specifications.
  if (is_string($display)) {
    // Allow modules to change the view mode.
    $entity = array($file->fid => $file);
    sdpm($entity);
    $view_mode = key(entity_view_mode_prepare('file', $entity, $display, $langcode));
    $display = file_display($file->type, $view_mode);
  }
  else {
    $view_mode = '_custom_display';
  }

I see the file with a .numbers extension has undefined type and confused filemime as application/octet-stream. Which means that .numbers extension files probably need to be caught during file upload and be applied correct type and mime. On the other hand, with rapid development of technologies all types of new extensions could be introduced in the future, so there should be some kind of fallback for all undefined file types.

Screenshot 2024-08-02 at 11 48 04 AM
avpaderno commented 1 month ago

For an unknown file type and a file that does not contain text, application/octet-stream is probably the best guess.
That guess is still helpful, as it would still be possible to render a file without knowing its file type, using a generic renderer. (I can imagine rendering a file that contains only text as text, although it should be sanitized to be put in HTML markup, and an application/octet-stream file as sequence of hexadecimal values.)

The issue is that there are not default renders for unknown file types.

In this case, the possible work-around could be having a field for renderable files, which are rendered, and a field for files which cannot be rendered, which are shown differently (for example, showing the filename). It would be a bit confusing for people using the site, though.

alanmels commented 1 month ago

@avpaderno, as yourself have said it would be confusing. I believe the core must be as fool-proof as possible. We really cannot expect users to be creating two different fields for two use-case file types. The core should have a fallback solution for files like .numbers and, in fact, any kind of other unknown types.

avpaderno commented 1 month ago

@alanmels I agree. My previous comment was meant to say that the work-around is not practical. Imagine moving a file from a field to another because it became possible to render that file type.

alanmels commented 1 month ago

Until this one is decided, I've used the following code:

$display = array(
    'formatter' => 'file_field_file_download_link',
    'settings' => array(
      'file_field_file_download_link' => array(
        'text' => 'Download [file:name]',
      ),
    ),
  );
  file_display_save('undefined', 'default', $display);

and it created a config file with the following content:

alan@mhb-web:/var/www/html/docroot$ cat files/config_c5cb71ee8153aa188fed97e8b8c14b4b/active/file_display.undefined.json 
{
    "_config_name": "file_display.undefined",
    "type": "undefined",
    "default": {
        "formatter": "file_field_file_download_link",
        "settings": {
            "file_field_file_download_link": {
                "text": "Download [file:name]"
            }
        }
    }
}

and it took care of the error notice.

argiepiano commented 1 month ago

@alanmels thanks for the new steps. I've been able to reproduce. I will take a closer look at this problem and your workaround over the weekend and will share feedback.

argiepiano commented 1 month ago

Can you please specify where you are using the code above?

alanmels commented 1 month ago

Oh, I just used a hook_init() function once and then deleted it as once the config file is created, we don't need execute the code in that particular website, so it's just a temp solution for one of our websites. The issue must be addressed properly at core, though.

argiepiano commented 1 month ago

@alanmels, I've looked a little into this.

While I agree that getting a warning is not good, the TWO deeper problems here are that:

  1. Backdrop doesn't "know" the mimetypes or file extensions for Apple Numbers files, AND
  2. The mimetype application/octet-stream (which is what you got when you upload a file with an unknown mimetype) is not "linked" to any File Type (neither Document, Audio, Video or Image types).

You can actually manually add mimetype application/octet-stream to the list of mimetypes accepted by the Document file type at admin/structure/file-types/manage/document. You would need to then manually fix the column type in table file_managed for all previously-uploaded numbers files.

This is what I propose:

  1. Adding application/octet-stream to the accepted mimetypes for Document file type by default. This will effectively fix this problem for the future: any unknown mimetype that is mapped as octet-stream will be considered a Document, and will have its own display formatter. I'd like to get more feedback from @backdrop/core-committers about this.

  2. In addition, adding Apple mimetypes and extensions to file_default_mimetype_mapping(), and adding them to the mimetypes accepted by Document file types. I feel that Apple files (numbers, keynote and pages) have been around for long enough and are used widely. I'll open a separate issue for this.

argiepiano commented 1 month ago

@alanmels, I've created two issues related to this:

@alanmels can you review and test #6680? That will speed up the merging.

argiepiano commented 1 month ago

@alanmels a quick way to reclassify file types AFTER you've manually added the octet-stream mimetype to a file type (e.g. to document) is by visiting admin/structure/file-types/classify. This will look up the mime and match it to a file type. This will save you the time of manually assigning file types to previously uploaded numbers files (or other octet-stream mimetype files).