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

Comments

marcoscano created an issue. See original summary.

marcoscano’s picture

Assigned: marcoscano » Unassigned
Status: Active » Needs review
FileSize
1.58 KB
markdorison’s picture

Status: Needs review » Reviewed & tested by the community
YesCT’s picture

Status: Reviewed & tested by the community » Needs review

This 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.

marcoscano’s picture

I 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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

  • alexpott committed 3cd528f on 8.4.x
    Issue #2782555 by marcoscano, YesCT, Berdir: FileUsageInterface::add()...

  • alexpott committed e4210af on 8.3.x
    Issue #2782555 by marcoscano, YesCT, Berdir: FileUsageInterface::add()...

Status: Fixed » Closed (fixed)

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