Problem/Motivation

\Drupal\file\Entity\File doesn't honor some type hints in \Drupal\file\FileInterface e.g the file size could be null because "the file itself might not exist or be available right now" but getSize() is supposed to return a string. Meanwhile setSize() accepts an integer, so the type for getSize() changes between string and integer depending on if you're getting from the database or from an entity object that is being built/modified. Likewise, the MIME type could be null if the MIME type guesser was unable to find a MIME type.

Steps to reproduce

You can create a file programmatically to play around with its methods:

$file = \Drupal\file\Entity\File::create(['created' => NULL]);
$file->getCreatedTime();
$file->getFileUri();
$file->setFileUri('lol');
$file->save();
$file = \Drupal\file\Entity\File::load($file->id());

Proposed resolution

Fix \Drupal\file\FileInterface type hints to also accept or return null where null is accepted and saved to the database:

  • getCreatedTime()
  • getFilename()
  • setFilename()
  • getMimeType()
  • setMimeType()
  • getSize()
  • setSize()

Fix \Drupal\file\FileInterface type hint to allow returning null where null cannot be saved to the database, but may be the current value because it hasn't been saved yet:

  • getFileUri()

Fix \Drupal\file\Entity\File method to ensure it returns an integer (or null) rather than a string, which is already documented to return an integer:

  • getCreatedTime()

Fix \Drupal\file\Entity\File method to ensure it returns an integer (or null) rather than a string, which previously was documented to return a string, while its setter accepts integer:

  • getSize()

Remaining tasks

In a followup issue targeting a future major version, the phpdoc type hints could be "upgraded" to actual type declarations for function arguments and return types.

API changes

File::getCreatedTime() and File::getSize() now return an integer (or null) rather than a string, which is useful if you want to use them with PHP functions that expect integers (you will still have to check if they are null, however). FileInterface::getCreatedTime() was already documented to return an integer anyways.

Data model changes

None.

Comments

mfb created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, file-type-hints.patch, failed testing. View results

mfb’s picture

Status: Needs work » Needs review
StatusFileSize
new5.16 KB

Fix test assertions to expect integer size rather than string size.

Status: Needs review » Needs work

The last submitted patch, 3: file-type-hints-3262358-3.patch, failed testing. View results

mfb’s picture

Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new5.41 KB
new1.77 KB

Fix test assertions to expect integer created rather than string created.

mfb’s picture

Version: 10.0.x-dev » 10.1.x-dev
smustgrave’s picture

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. 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 require as a guide.

First thank you for a great issue summary.

Verified patch still applies to 10.1.x
Checked what was written in the IS matches the patch.

As for the remaining task since this is already complete that task can be a follow up?

+1 for RTBC but will get another thought on that remaining task.

joachim’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/FileInterface.php
@@ -91,7 +91,7 @@ public function setMimeType($mime);
-   * @return string
+   * @return int|null
    *   The size of the file in bytes.

All these docs that are being changed to TYPE|null need to say what a return of NULL means.

> I t's probably a good idea to add actual type declarations for function arguments and return types at this time as well?

IIRC there is a meta-issue for adding return types to existing functions -- check what that says about BC?

EDIT: Found it - it's #3050720: [Meta] Implement strict typing in existing code and it's complicated. So I don't think we can add types here.

mfb’s picture

Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new2.48 KB
new5.98 KB

Updated the phpdoc to document what NULL means; updated issue summary to say that actual type declarations could potentially be added in a followup, and to clarify that this patch contains File API changes (in addition to FileInterface documentation changes). I'm not sure what the policy is around such "bugfix API changes" happening in minor versions - I had initially targeted this issue for 10.0 major version.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @mfb for the additional text!

Sorry this got lost in my mail didn't mean for it to sit so long.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1c96bea and pushed to 10.1.x. Thanks!

  • alexpott committed 1c96bea2 on 10.1.x
    Issue #3262358 by mfb, smustgrave, joachim: Fix type hints in...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.