Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The documentation for the method ::add() of the FileUsageInterface states:
/**
* Records that a module is using a file.
*
* Examples:
* - A module that associates files with nodes, so $type would be
* 'node' and $id would be the node's nid. Files for all revisions are
* stored within a single nid.
* - The User module associates an image with a user, so $type would be 'user'
* and the $id would be the user's uid.
*
* @param \Drupal\file\FileInterface $file
* A file entity.
* @param string $module
* The name of the module using the file.
* @param string $type
* The type of the object that contains the referenced file.
* @param int $id
* The unique, numeric ID of the object containing the referenced file.
* @param int $count
* (optional) The number of references to add to the object. Defaults to 1.
*/
public function add(FileInterface $file, $module, $type, $id, $count = 1);
The documentation for the parameter $id (refering to an int value) does not correspond to its schema:
/**
* Implements hook_schema().
*/
function file_schema() {
$schema['file_usage'] = array(
'description' => 'Track where a file is used.',
'fields' => array(
// ...
'id' => array(
'description' => 'The primary key of the object using the file.',
'type' => 'varchar_ascii',
'length' => 64,
'not null' => TRUE,
'default' => 0,
),
// ...
The same applies for the ::delete() method.
Proposed resolution
Change the docblock to inform that the parameter $id can be any string.
Remaining tasks
- Review patch
User interface changes
- None
API changes
- None
Data model changes
- None
Comment | File | Size | Author |
---|---|---|---|
#2 | fix-documentation-of-fileusageinterface-2782555-2.patch | 1.58 KB | marcoscano |
Comments
Comment #2
marcoscanoComment #3
markdorisonComment #4
YesCT CreditAttribution: YesCT commentedThis change makes me a bit uneasy.
Maybe we do really want people only passing in integers (or strings that represent integers?)
I looked at git blame for those lines and they are from #1797478-26: Make file usage storage pluggable and may not have gotten a type review.
I think more of a review is needed, perhaps looking at usages of those methods and seeing what types are actually being passed in would help.
Comment #5
marcoscanoI came across this when I saw that some contrib modules pass in a string to this value, so I assumed that what was wrong was the documentation and not the schema definition.
A quick search in core shows that most of the times a numeric id is used, but sometimes a uuid is used also.
IMHO, it could make sense to allow an arbitrary string, once it may be legitimate that a file is being used by something else than a standard entity.
Comment #7
BerdirAgreed, this is a bug, core clearly uses with strings as well, implicitly and explicitly (we support content entities with string ID's, in which case image/file fields would also use strings).
So in theory this is an API change for alternative implementations, but they would fail right now anway.
Comment #8
alexpottCommitted and pushed 3cd528f to 8.4.x and e4210af to 8.3.x. Thanks!
Thanks @YesCT and @Berdir for helpful comments.
I've backported this to 8.3.x because as @Berdir all implementations need to handle strings and also with respect to primitive types like string and integer whilst we support php5.5.9 we have no way to enforce this.