Skip to content

Let's not just use file_get_contents() for better compat.#4924

Merged
samhotchkiss merged 5 commits intomasterfrom
tonesque/file_get_contents
Oct 11, 2016
Merged

Let's not just use file_get_contents() for better compat.#4924
samhotchkiss merged 5 commits intomasterfrom
tonesque/file_get_contents

Conversation

@georgestephanis
Copy link
Copy Markdown
Contributor

No description provided.

@jeherve jeherve added Bug When a feature is broken and / or not performing as intended [Feature] Theme Tools [Status] Needs Review This PR is ready for review. labels Aug 23, 2016
@jeherve jeherve added this to the 4.2.3 milestone Aug 23, 2016
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Aug 23, 2016

This might cause some issues in some cases, as mentioned here:
#3742 (comment)

We could use wp_remote_get() if the $image_url is prefixed by http:// or https:// however would need to use file_get_contents() for all other protocols and image paths anyway (ie. /home/.../image.jpg or custom-file-wrapper://path/image.jpg)

@georgestephanis
Copy link
Copy Markdown
Contributor Author

georgestephanis commented Aug 23, 2016

Ah, thanks, I hadn't seen that.

The function name does explicitly have from url in the name, so specifying a local path doesn't seem like it would be done often? But I'd be happy to add in caveats for that as well for better compat and fewer http requests.

Also other possible place --

$g_gif = file_get_contents( 'https://pixel.wp.com/g.gif?v=wpcom-no-pv&x_likes=disabled_likes' );

@lezama lezama modified the milestones: 4.3.1, 4.2.3 Sep 2, 2016
@eliorivero eliorivero added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Sep 2, 2016
@samhotchkiss samhotchkiss modified the milestones: 4.2.3, 4.3.1 Sep 2, 2016
@eliorivero eliorivero added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Review This PR is ready for review. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 2, 2016
@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented Sep 2, 2016

Works fine with URLs, images are recognized, even with no extensions. Non images return false, which is correct too.
Loading through filesystem isn't working though.

I tested with something like

add_action( 'wp_footer', function() {
    $ton = new Tonesque( 'http://example.tld/wp-content/uploads/2029/08/image.jpg' );
    var_dump( $ton->grab_color() );
} );

and it works, you get the array with r, g and b keys. But, if you replace the URL with a path like /Users/someuser/Sites/someserver/wp-content/uploads/2029/08/image.jpg it doesn't work.

@lezama
Copy link
Copy Markdown
Contributor

lezama commented Sep 2, 2016

Personal opinion: I think we should add another method imagecreatefromfile instead of complicating things in imagecreatefromurl

@rcoll
Copy link
Copy Markdown
Member

rcoll commented Oct 4, 2016

It appears that in PHP7, list() must be fed an integer-keyed array. I received the following when loading up Tonesque with a local file path:

Notice: Undefined offset: 0 in /home/jetpack-single.ftlwp.com/wp-content/plugins/jetpack/_inc/lib/tonesque.php on line 69 Notice: Undefined offset: 1 in /home/jetpack-single.ftlwp.com/wp-content/plugins/jetpack/_inc/lib/tonesque.php on line 69 Warning: imagecreatefromstring(): Empty string or invalid image in /home/jetpack-single.ftlwp.com/wp-content/plugins/jetpack/_inc/lib/tonesque.php on line 76

I just pushed a commit that should resolve this. Otherwise, everything LGTM and tests good.

@samhotchkiss samhotchkiss added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 11, 2016
@eliorivero eliorivero removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Oct 11, 2016
@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented Oct 11, 2016

LGTM! 🐑 Tested again with Ryu theme and this code and it works fine now

add_action( 'wp_footer', function() {
    $ton = new Tonesque( 'http://example.tld/wp-content/uploads/2029/08/image.jpg' );
    var_dump( $ton->grab_color() );

    $ton = new Tonesque( '/Users/joedoe/Sites/joeserver/wp-content/uploads/2029/08/image.jpg' );
    var_dump( $ton->grab_color() );
} );

@samhotchkiss samhotchkiss merged commit efa0a7c into master Oct 11, 2016
@samhotchkiss samhotchkiss deleted the tonesque/file_get_contents branch October 11, 2016 20:18
@samhotchkiss samhotchkiss removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Theme Tools Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants