Problem/Motivation

While working on #2928904: Add a mapping target to media field and seeing several file related issues, I think it make sense to move the code for resolving a file to a separate service. This way the Media target can make use of the file resolving logic without needing to extend the File class.

The downside is that this does introduce a BC break. Modules that were extending the File class to resolve file related issues would now need to decorate the FileResolver service.

Proposed resolution

Add a FileResolver service and adjust the File target to make use of this service. Deprecate methods in the File class that are related to resolving files.

Remaining tasks

  • Review
  • Merge

User interface changes

None.

API changes

Several methods in \Drupal\feeds\Feeds\Target\File will no longer be used and get deprecated:

  • getFileName()
  • getContent()
  • writeData()

Modules that overrode these methods could break.

Data model changes

None.

Issue fork feeds-3565186

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

megachriz created an issue. See original summary.

megachriz’s picture

Status: Active » Needs review

megachriz’s picture

I've done some testing and reviewing the code this week.

Latest changes:

  • A BC break is avoided: when subclasses override the File target methods getFileName(), getContent() or writeData() a fallback to the old implementation will be followed. If we would leave out the fallback, then these methods would no longer get called if subclasses overrode them. And that possible would break workflows. The methods are still deprecated, though. So the fallback won't be there forever.
  • A change record for this issue has been drafted.

Findings so far on reviewing/testing the code:

  • When downloading a file without no extension, that file gets saved to file system - and then importing the entity fails because of validation errors, while previously the download was aborted - and the entity got imported. I plan to address this in #3076946: Failed to validate remote image with no file extension, which I already have worked on.
  • With FileResolver, other protocols than "http" or "https" are no longer supported. In the previous implementation other protocols didn't work out of the box, but you could do so by adding the following to settings.php:
    $settings['http_client_config']['curl'] = [
      CURLOPT_PROTOCOLS => CURLPROTO_HTTP | CURLPROTO_HTTPS | CURLPROTO_FILE | CURLPROTO_FTP,
    ];
    

    I did not manage to get a successful import with a file specified with either file:// or ftp://, so I'm going to assume it is a far fetched edge case. We could re-add support for other protocols once users really need it.

megachriz’s picture

Status: Needs review » Fixed

I scheduled merging the code!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • megachriz committed 6038e152 on 8.x-3.x
    feat: #3565186 Added service for resolving files.
    
    By: megachriz
    

Status: Fixed » Closed (fixed)

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