ResponsiveImagesCG / wp-tevko-responsive-images

Fully responsive image plugin for wordpress
449 stars 53 forks source link

Consider path filtering for images in content. #190

Closed joemcgill closed 9 years ago

joemcgill commented 9 years ago

When we filter the HTML of images in posts, we only do so if the image path matches the value of wp_uploads_dir(). We may need to keep in mind cases where a developer would want to filter images that have been uploaded somewhere else (e.g. S3).

See: https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/pull/177#discussion_r40546855

tevko commented 9 years ago

If an image were uploaded elsewhere, could we reasonably infer that other sizes exist?

joemcgill commented 9 years ago

cc: @bradt

bradt commented 9 years ago

@tevko I don't understand the question.

joemcgill commented 9 years ago

@bradt I believe he's asking if it's possible to identify the Attachment ID of an image that's been moved offsite, when the uploads directory remains on the server. And if not, how would we determine the other available image sizes to include in the srcset attribute?

joemcgill commented 9 years ago

We should try to address this before a core merge, if possible.

peterwilsoncc commented 9 years ago

wp_upload_dir['baseurl'] is filterable, so a plugin offloading images to a CDN/S3 should probably change the URL before it hits the regular expression in tevkori_filter_content_images.

The full filter in wp_upload_dir is:

/**
 * Filter the uploads directory data.
 *
 * @since 2.0.0
 *
 * @param array $uploads Array of upload directory data with keys of 'path',
 *                       'url', 'subdir, 'basedir', and 'error'.
 */
$uploads = apply_filters( 'upload_dir',
    array(
        'path'    => $dir,
        'url'     => $url,
        'subdir'  => $subdir,
        'basedir' => $basedir,
        'baseurl' => $baseurl,
        'error'   => false,
    ) );
joemcgill commented 9 years ago

That's what I was thinking too, but I wasn't sure if @bradt had a use case where this wouldn't be the case.

bradt commented 9 years ago

Yes, there is, as I described here: https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/pull/177#discussion_r40578482

bradt commented 9 years ago

I'll lay it out in more detail...

Let's say you have a site with a bunch of existing media, but you want to start offloading any NEW media to S3. You could install WP Offload S3 (free version) and do just that. Now your new content will have S3 URLs and old content will have URLs to your server.

In that case, you're going to want RICG to look for both S3 URLs and URLs to your local server.

joemcgill commented 9 years ago

Important to get this one right. Easier to add a filter than move it later, so I'm moving it off this milestone for now.

peterwilsoncc commented 9 years ago

I'm glad you caught this, it occurred to me after the event filtering the regex would have an affect on the matches array & may cause problems with reset of the function.

Some random thoughts:

joemcgill commented 9 years ago

change the callback so it accepts the entire image tag as a string and works from that

This is exactly what I've been thinking. Currently, the callback only works if we can parse the id from the HTML, so it shouldn't matter what the src is. Now that we're no longer using a preg_replace_callback() it makes less sense for us to use a callback, and a utility function that accepts a full img element as a string and returns the HTML with srcset and sizes added, would be way more useful.

I'll add a PR with the code for how this could work shortly.