Follow-up to #2623012: Implement interfaces and base classes for URL-based sources

The only fetcher currently implemented is for HTTP - we need one for processing local files.

I needed a File Data Fetcher as my XML files are local.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pguillard created an issue. See original summary.

pguillard’s picture

Status: Active » Needs review
FileSize
1.77 KB
kriboogh’s picture

Thanks for making this into a patch (https://www.drupal.org/node/2779083)...

rodrigoaguilera’s picture

Title: File Data Fetcher » Implement file-based fetcher
Issue summary: View changes
Parent issue: » #2623012: Implement interfaces and base classes for URL-based sources
Related issues: +#2640510: Implement file-based fetcher
mikeryan’s picture

Status: Needs review » Needs work

As I contemplate #2802447: Fetcher plugin for sftp, given there are stream wrappers for sftp and related protocols, I look here and see file_get_contents, which of course can read from any stream wrapper. I think with just a little tweaking, this plugin can work generally with simple stream wrappers, and then we only need to use specialized fetchers like http when we need to pass options beyond a simple URL. And assuming this does work for local files/HTTP/etc., it should be worth making this the default fetcher for the url source plugin, so it won't need to be specified.

Since I need this for my current project, I'll pick it up from here today/tomorrow.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
1.62 KB

Here we go - I've tested this version of the fetcher plugin with a local absolute path, a path relative to the Drupal root, an http URL, and (my main motivation) a ssh2.sftp URL. Seems to work fine...

mikeryan’s picture

  • mikeryan committed 41f9efc on 8.x-2.x authored by pguillard
    Issue #2781883 by pguillard, mikeryan: Implement file-based fetcher
    
mikeryan’s picture

Status: Needs review » Fixed

Committed with a bit more tweaking, thanks!

mchelen’s picture

#6 works for me, thanks!

heddn’s picture

  1. +++ b/src/Plugin/migrate_plus/data_fetcher/File.php
    @@ -0,0 +1,58 @@
    +  public function setRequestHeaders(array $headers) {
    +    // Does nothing.
    ...
    +  public function getRequestHeaders() {
    +    // Does nothing.
    +    return [];
    

    At this point, maybe it doesn't matter, but why are we requiring no-op methods like these in the data fetcher interface? Maybe that's for a follow-up.

  2. +++ b/src/Plugin/migrate_plus/data_fetcher/File.php
    @@ -0,0 +1,58 @@
    +      throw new MigrateException('Error message: ' . $e->getMessage() . ' at ' . $url .'.');
    

    Nit: the doxygen doesn't mention that it can throw an exception. Is that something we want to include in its docs?

  3. This should also have tests, which should be easy enough.
mikeryan’s picture

At this point, maybe it doesn't matter, but why are we requiring no-op methods like these in the data fetcher interface? Maybe that's for a follow-up.

Yep, going to follow-up by finally dealing with #2640512: Make custom headers an HTTP-specific feature.

Nit: the doxygen doesn't mention that it can throw an exception. Is that something we want to include in its docs?

Good point for a quickfix.

This should also have tests, which should be easy enough.

Everything needs tests:P.

Status: Fixed » Needs work

The last submitted patch, 6: implement_file_based-2781883-6.patch, failed testing.

mikeryan’s picture

Status: Needs work » Fixed
kriboogh’s picture

Status: Fixed » Closed (fixed)

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