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.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | file-type-hints-3262358-9.patch | 5.98 KB | mfb |
Comments
Comment #3
mfbFix test assertions to expect integer size rather than string size.
Comment #5
mfbFix test assertions to expect integer created rather than string created.
Comment #6
mfbComment #7
smustgrave commentedThis 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.
Comment #8
joachim commentedAll 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.
Comment #9
mfbUpdated 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.
Comment #10
smustgrave commentedThank you @mfb for the additional text!
Sorry this got lost in my mail didn't mean for it to sit so long.
Comment #11
alexpottCommitted 1c96bea and pushed to 10.1.x. Thanks!