Skip to content
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

Change media handling to only process featured image by default #184

Merged
merged 14 commits into from
Sep 17, 2018

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Aug 17, 2018

Currently, when an item is distributed, we process all attached images to the post object and download those to the receiving site. But only the featured image is actually replaced. All other images (i.e. in content images) still reference the original site URL.

Typically the downloading of these images will be the biggest performance issue when distributing content, and if those images aren't actually being used, as is the case currently, it's an unnecessary performance burden.

Based off discussion in #161, this PR changes the default behavior to only process the featured image by default. It adds a new setting to the plugins settings page where you can change that to be featured image and all attached images, which is the current default.

I've also gone ahead and added some additional filters around media handling, both to standardize the filter we already had in place for internal connections and allow more areas where media handling can be turned off entirely.

Darin Kotter added 7 commits August 10, 2018 13:33
…image processing from normal media processing.
…new filter around setting media on pulled internal posts, to match the one we have on pushed internal posts.
…red image only but can select featured plus attached. Make sure this value sets the proper filter, allowing developer control over the media handling.
… the set_media function, to only use the featured image by default. Doing it on the set_media function allows whatever site is actually receiving the content to decide what to do with images, instead of the sending site.
@helen helen requested a review from tlovett1 August 28, 2018 19:47
@dkotter
Copy link
Collaborator Author

dkotter commented Aug 28, 2018

Screenshot of new setting:
screen shot 2018-08-28 at 1 51 17 pm

@tlovett1
Copy link
Member

Hey @dkotter, I just tested this. When I chose process all media, still only the featured image seemed to be distributed.

I also wonder about the setting language. Maybe instead of "process" it's "distribute"? CC @jakemgold

Darin Kotter added 3 commits August 31, 2018 09:25
…ge in our media array that we still process that. If we don't have any featured images, make sure we set our media array to empty.
@dkotter
Copy link
Collaborator Author

dkotter commented Aug 31, 2018

@tlovett1 Just tested this again (both internal and external Push and Pull) and I did run into a few issues, which I've fixed (issue where if there was no featured image, it still processes all images, even if you don't want. And an issue where if the featured image was the first image in the media array, it wouldn't process that), but never saw the issue of only getting the featured image, when I have the setting for featured and attached selected.

One thing to note here though is that to properly test this, these changes need to be installed on both sites that are doing the distribution. The way this is set up, the site that is ingesting the content determines what to do with images, so whatever setting is on that site is what impacts this.

For instance, if I push from Site A to Site B, Site A will send all images across, like it does now. Site B then determines what to do with those images, based on this media handling setting. It will either download just the featured image, if there is one, or all images that were sent.

If I pull from Site B into Site A, Site B will send all images and Site A will determine what to do with those images, based on the media settings it has in place.

So depending on how you're testing this, you'll need to make sure these changes are installed on both sides and the settings for both are set the way you want to test. Also note, we currently pull attached images from a post, but an image can't be attached to more then one post. This caught me a few times in testing, where I was using the same image across multiple posts but WordPress attached that image to the first post I inserted it into, so the other posts weren't sending that as an attached image. That's an issue we discussed as a phase 2 approach here, to make sure we send all images a post references, attached or not.

Also, for the wording, happy to update that as needed but again, those settings don't change how images are distributed, it changes how images are downloaded/ingested/processed.

includes/settings.php Outdated Show resolved Hide resolved
@helen helen added this to the 1.3 milestone Sep 13, 2018
@helen helen merged commit 3839f17 into master Sep 17, 2018
@helen helen deleted the feature/media-handling branch September 20, 2018 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants