Firtst off: I love this module!
I have created a plugin where this module fetches remote content from all out local sites (indexed in elasticsearch). This module makes life so much easier!
Thanks.

We use a proxy server in our network and all our outgoing requests should go through that proxy.
Therefor the drupal Client should always be used when doing outgoing requests (or you should implement support for the proxy settings) .
Read more info here: https://www.drupal.org/node/2637058

So in Drupal\filefield_sources\Plugin\FilefieldSource\Remote.php line 69

      $ch = curl_init();
      curl_setopt($ch, CURLOPT_URL, $url);
      curl_setopt($ch, CURLOPT_HEADER, TRUE);
      curl_setopt($ch, CURLOPT_NOBODY, TRUE);
      curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
      curl_setopt($ch, CURLOPT_HEADERFUNCTION, array(get_called_class(), 'parseHeader'));
      // Causes a warning if PHP safe mode is on.
      @curl_setopt($ch, CURLOPT_FOLLOWLOCATION, TRUE);
      curl_exec($ch);
      $info = curl_getinfo($ch);
      if ($info['http_code'] != 200) {
        curl_setopt($ch, CURLOPT_HTTPGET, TRUE);
        $file_contents = curl_exec($ch);
        $info = curl_getinfo($ch);
      }
      curl_close($ch);

should be replaced by

$client = \Drupal::httpClient();
try {
  $request = $client->get($url);
  $status = $request->getStatusCode();
  $file_contents = $request->getBody()->getContents();
}
catch (RequestException $e) {
  //An error happened.
}

Also Line 163

        $ch = curl_init();
        curl_setopt($ch, CURLOPT_URL, $url);
        curl_setopt($ch, CURLOPT_HEADER, FALSE);
        curl_setopt($ch, CURLOPT_WRITEFUNCTION, array(get_called_class(), 'curlWrite'));
        // Causes a warning if PHP safe mode is on.
        @curl_setopt($ch, CURLOPT_FOLLOWLOCATION, TRUE);
        $transfer_success = curl_exec($ch);
        curl_close($ch);

Should be replaced by something like

$client = \Drupal::httpClient();
try {
  $request = $client->get($url);
  $status = $request->getStatusCode();
  $transfer_success = $request->getBody()->getContents();
}
catch (RequestException $e) {
  //An error happened.
}

But I don't understand what the CURLOPT_HEADERFUNCTION does.

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wouters_f created an issue. See original summary.

wouters_f’s picture

Issue summary: View changes

* patch on the way...

wouters_f’s picture

The uploading using the "remote" filefield source works with these changes.
I'm not really sure what to do in the error cases. Any advice would be welcome.

wouters_f’s picture

Assigned: wouters_f » Unassigned
Status: Active » Needs review
oriol_e9g’s picture

Status: Needs review » Needs work

New code break tests, still needs work, $info var is removed and used later.

KevinVanRansbeeck’s picture

KevinVanRansbeeck’s picture

jonathanshaw’s picture

+++ b/src/Plugin/FilefieldSource/Remote.php
@@ -53,41 +53,31 @@ class Remote implements FilefieldSourceInterface {
-      // Update the $url variable to reflect any redirects.
-      $url = $info['url'];

We seem to have lost that functionality, but in pirnciple it still seems relevant. Maybe it could be accessed from that request instead.

w.drupal’s picture

@jonathanshaw

Seems it's not relevant as well as a second call via HTTP client. Even if the source path will return 301 redirect the client will process it and return response from the redirected page. Let me know if i'm wrong but it what my tests showed. Also i added:

  1. RequestException to the use section
    @@ -4,6 +4,7 @@ namespace Drupal\filefield_sources\Plugin\FilefieldSource;
    
     use Drupal\Core\Form\FormStateInterface;
     use Drupal\filefield_sources\FilefieldSourceInterface;
    +use GuzzleHttp\Exception\RequestException;
     use Symfony\Component\Routing\Route;
     use Drupal\Core\Field\WidgetInterface;
     use Symfony\Component\HttpFoundation\JsonResponse;
    
  2. Process $e->getCode() in the catch section
rivimey’s picture

Looks to me like a reasonable patch. The old code did the equivalent of a HEAD call on the URL and checked the returned headers before (possibly) retrying with a GET; the new code just does the GET straight away. I'm not sure why the old code wanted a distinct HEAD, but it does seem unnecessary to me.

From my POV this passes a visual review, though I haven't applied the patch to actually test it (yet).

marassa’s picture

Status: Needs review » Reviewed & tested by the community

I encountered warnings about wrong calls of curl-related functions after I switched to PHP 8: Warnings while adding a remote file after switching to PHP 8.0. Applying patch #9 getting rid of direct curl calls turned out easier than trying to figure out the existing code and solved the problem. Recommended.

Kris77’s picture

Patch #9 works for me too.
Thanks @w.drupal

rschwab’s picture

Since php 8 is required for Drupal 10, I'm marking this as a child of #3336268: Drupal 10 compatibility to try and get that ticket completed. This potentially solves the last errors in the D10 test suite.

rschwab’s picture

Status: Reviewed & tested by the community » Needs review

Applied #9 to an issue fork, then removed the legacy curl functions that are no longer in use. Currently passing all tests, but needs community review.

rschwab’s picture

Version: 8.x-1.x-dev » 2.0.x-dev
szato’s picture

Status: Needs review » Needs work
FileSize
96.38 KB

There is a fatal php error because of duplicate use statements.

I'm going to rebase the MR8 on 2.0.x and fix these.

szato’s picture

Status: Needs work » Needs review

Please review it, now it works ;)