-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
generateSprite does not sideload images #117
Comments
Henrique provided an more concise example: // Create a temporary image. unlink call is required on the end
$temp_image = wp_tempnam();
$some_filename = ...
// Do the image operations
$image = imagecreatefrompng( $temp_image );
// do image operations
// (...)
// Save the image to the temporary file
imagepng( $image, $temp_image );
$file_array = [
'name' => $some_filename,
'tmp_name' => $temp_image,
];
// Sideload the image
$attachment_id = media_handle_sideload( $file_array, 0 );
// destroy the image
imagedestroy( $image );
// unlink the temporary file
if ( file_exists( $temp_image ) ) {
unlink( $temp_image );
} |
I've started work on this in #126. I don't yet understand enough about what sprites are used for/how they interact with other parts of the system to determine what additional work needs to be done. |
#126 should be all that is required to resolve this. The generated sprite will be sideloaded and thus available on the CDN. I didn't see matching code in the referenced watermark plugin and I am not certain about the reference to |
OK, I think I see how to fix this... |
Hi @yo35 - thanks for the feedback. Yes, I did see how this feature was used. In my local testing, the code as provided in https://github.com/yo35/rpb-chessboard/pull/126/files does work as far as uploading the images, and they show up correctly in the admin. I didn’t get as far as adding all the pieces and then testing on the front end, however I can do that and provide some screencasts of my testing if that is helpful. A bit of background about why we are proposing these changes, and how the changes work... In your original code, you generate a path for the sprite file in The pull request seeks to address this: first we use Based on your comments, I think the missing piece here is storing the resulting media object id so we can reference that later in |
Hi @adamsilverstein. |
Yes, I was concerned about that as well. The way I have written it, for each sprite the code looks for the stored sprite URL (based on the image url) for the sprite and if it does not find it, it falls back to the current method - so I think it will work correctly when 'migrating'. I agree it would be worth testing an upgrade to verify this is true, I haven't had a chance to try that yet... Please let me know if you are able to test this - thanks! |
Hi @paulschreiber and @adamsilverstein, At the end, I've come to a more radical solution than the one @adamsilverstein suggested in PR #126, which is to change the sprite format, in order to avoid the need for images such as Beside solving this "sideload" problem, this solution has two benefits:
The only drawback (as far as I see) is the resort to CSS attribute The fix is available in master. |
Hey @yo35 Thanks for addressing that! Very cool to see you make such a bold update, I agree this seems fundamentally like an even better approach. We will review the latest version and let you know if we have additional feedback. Thanks again! |
WordPress VIP support will not allow this plugin to be used until this issue is addressed:
models/ajax/formatpiecesetsprite.php
On the
generateSprite
method, the image is not being sideloaded, which means that will not work properly as it won't be copied to the CDN. Should be usedmedia_handle_sideload
orwp_upload_bits
alongside withapply_filters( wp_handle_upload, $file_upload )
to create a new attachment. You can take a look at the Watermark plugin for examples: https://vip-svn.wordpress.com/plugins/watermark-image-uploads/watermark-image-uploads.phpThe text was updated successfully, but these errors were encountered: