generateSprite does not sideload images

[paulschreiber]

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 used media_handle_sideload or wp_upload_bits alongside with apply_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.php

[paulschreiber]

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 );
}

[paulschreiber]

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.

[adamsilverstein]

#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 apply_filters( wp_handle_upload, $file_upload ) - I am not sure we need that part and will check with VIP

[yo35]

Hi,
Unfortunately, it does not work. And I don't understand why, since I don't understand at all what the media_handle_sideload() function is supposed to do. And it is even worse after reading the documentation ("Handles a sideloaded file in the same way as an uploaded file is handled by media_handle_upload()", what does that mean ??).

Let me describe the process in which this generateSprite() function is used:

1. In the admin, 'Chess' > 'Theming' > 'Piecesets' > 'Add a new pieceset' > click on one of the image with the question mark sign:
   
   => This action opens the media upload popup. The user is asked to upload an image representing a chess piece -- a white queen in this example.

2. The user selects an image. It can be image that existed previously among the in the media library, or a newly uploaded image, it doesn't change anything for RPB Chessboard as there is no custom hook here.

3. At the end, the user clicks on 'Select'. This action invokes an AJAX routine on the server, which retrieves the image selected by the user at step 2) -- typically, an image like this:
   
   ... and generates another image out of it, looking like this:
   
   This second image is written on the file system during the AJAX call.

This is this generated image that is used to render the chessboard widget in page/post. In case of none-builtin piecesets, this image is referenced through its URL in the dynamically generated CSS templates/misc/theming.php. Therefore it is important to have somehow a public URL pointing at this image.

[yo35]

OK, I think I see how to fix this...

[adamsilverstein]

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 computeCustomPiecesetSpritePathOrURL which works fine a single WordPress install. In our case, however, the website is hosted in a distributed environment (across many servers), and the generated upload path does not come out correctly.

The pull request seeks to address this: first we use wp_tempnam(); to make a temporary file path, hen once the sprite is ready, we use media_handle_sideload to have WordPress import the file, brining it into the media library, this creates a file we can address in our distributed installation (and more importantly, from our image CDN).

Based on your comments, I think the missing piece here is storing the resulting media object id so we can reference that later in templates/misc/theming.php. I'm going to do some more testing to see if my understanding is correct.

[adamsilverstein]

@yo35 I pushed some additional changes in #126, can you please give this a test? I added storage of the imported url, so it can be used later.I verified this works correctly in my local testing, can you please review?

Thanks!

[yo35]

Hi @adamsilverstein.
Thanks for the explanation. I haven't had time to test your PR #126 in detail yet, although I think it should work for new installs.
However I'm a bit concerned with existing installs. I wonder whether it is possible to display a warning message to the admin when upgrading RPB Chessboard from a version that uses the "old" sprite location.

[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!

[yo35]

Hi @paulschreiber and @adamsilverstein,
First of all, thanks for your help and your valuable advices on this item.

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 . Now, each colored piece sprite is just a 64x64 pixel image, and it is resized on the fly by the browser when necessary.

Beside solving this "sideload" problem, this solution has two benefits:

• significant simplification of the custom pieceset form: there is no AJAX anymore, no need to maintain two versions of each custom sprite uploaded by the user, etc,
• standard sprite images are now much smaller (e.g. 23 kB instead of 615 kB for the cburnett set).

The only drawback (as far as I see) is the resort to CSS attribute background-size, which is not supported by IE 8. I think not supporting IE 8 is admissible in 2018 though!

The fix is available in master.

[adamsilverstein]

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!