Including remote images

[mhawker]

The is_file check on line 103 prevents the inclusion of remote files (eg, over http). However, file_get_contents will return false if either of the checks in line 103 fail. What do you think of changing base64($file) to read:

protected function base64($file)
{
    if (false === ($content = file_get_contents($file))) {
        throw new \InvalidArgumentException(sprintf('Cannot read file %s', $file));        
    }
    return base64_encode($file);
}

[krichprollsch]

Hello @mhawker thank you for your feedback.

As I can remember, we did'nt want to manage remote resources. The idea was to avoid path issues after build time. So remote resources was not in our scope.

I'm afraid this feature could introduce some BC breaks for users.

But maybe we could introduce an option to allow user to enable this feature. WDYT ?

[mhawker]

Hi Pierre, and thank you for getting back to me.

I spent some more time on this because I needed remote files in a project I'm working on. I ended up using guzzle for my project but I think it could be implemented here without too much difficulty.

There's another problem with using the library to read remote files: it seems (for me at least) that mime_content_type does not work for remote files; it would have to be replaced with another file_get_contents call that looks at the $http_response_header variable or uses a stream context.

I think it's not an insurmountable problem, but if it's out of scope then it's out of scope. Shall I do a PR and you can see what you think?

[krichprollsch]

I agree for a PR with a option to enable remote files processing :+1:

I don't want using something like guzzle, file_get_contents is fine.

Concerning the mime type mime_content_type is deprecated. We should use a better way to find the mimetype. Something which could works for both local and remote files would be great, but I don't find a good solution...

So maybe we could keep mime_content_type for now, and if you can use headers from http for the remote files, it's a good start.

[mhawker]

Hi @krichprollsch that sounds like a plan. I agree that adding Guzzle as a dependency is a little bit of overkill. I can't work on it this weekend (fathers day) but I'll have the PR ready toward the end of the week.

In regards to mime_content_type, I believe it's generally OK to use. It was deprecated in favor of the fileinfo extension, but it has been de-deprecated now and is part in the fileinfo extension. OTOH, there's no guarantee that the host has got mime magic installed. A fallback could download and parse the default Apache mime type map. Since there's a configuration file to distribute/generate, it will require some thought, but I think it's doable.

[mhawker]

OK, I think the PR is ready. Sorry that I submitted it a few times; this is my first one.

I'm a bit new at this; do you know how I can run the integration tests before bothering you with pull request?