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

Post thumbnail support #16

Merged
merged 4 commits into from Jan 10, 2018

Conversation

Projects
None yet
6 participants
@brocheafoin
Copy link
Contributor

commented Jul 6, 2017

This fixes issue #5 :

  • Thumbnail image copied into the aggregating blog's Media library, attached to synced post
  • Copied image set as post thumbnail for the corresponding post
  • If thumbnail is changed or removed in subsite, change is replicated on aggregating blog

Could be coded more cleanly. All the image stuff in its own class, for example, but I'm pretty sure my code is closer to the coding guidelines than what's already in there, so I'm OK with the current state of affairs. I would really like this to be merged, though, so I'll do what you want. Cheers!

Oh! I forgot. One or two methods were ripped from Automattic's aggregator, so contributors list should be modified accordingly.

brocheafoin added some commits Jun 29, 2017

Proper Thumbnails to Tags Site
First pass, nearly complete.
Todo: copy thumbnail when post_status changes, i.e. thumbnail added in
draft or private
Handle post status changes
Normally, feature complete.

Posts are only synced when they're 'publish', but thumbnails can be
added to a post in any status (most commonly, auto-draft). Thumbnails
should be synced to the tags blog when the status changes to 'publish'.
This is now handled.
@Darthmaul

This comment has been minimized.

Copy link

commented Jul 9, 2017

This is awesome! Great work! I will be testing this ASAP! :D

@tw2113

This comment has been minimized.

Copy link
Member

commented Jul 9, 2017

For information sake, see my comment at #5 (comment) for how, to certain degrees, featured images aren't completely broken with this plugin. They're just handled differently.

@brocheafoin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2017

Hi Michael,

featured images aren't completely broken with this plugin

I did not say that and if that's how you understood my message, I am really sorry. I had read the thread you point to before starting work. What it indicated to me was that people expected post thumbnails to be copied over to the aggregating blog. I know that's what I expected too. So I made it.

I understand there is some form of support, but it comes with a lot of drawbacks:

  • Need to write custom code in your theme to display thumbnails
  • Custom code needs to differentiate between aggregated and non-aggregated posts
  • By default, only one thumbnail size available
  • Need to write custom code to have access to more thumbnail sizes
  • Even then, thumbnail sizes are those of the aggregated blog, not the aggregating blog

What I'm proposing here removes all these drawbacks. You can now use WP native functions in the aggregating blog for all posts, no matter where they come from. Thumbnails are available in all sizes defined in the aggregating blog.

Now, I've probably been a bit agressive in removing the meta fields used in the current method. I'll add them back for backwards compatibility. Would that help?

@tw2113

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

Understood, and I wasn't offended or anything, just wanting to make sure it's clear to anyone coming by that may read it :)

I would definitely say strive for backwards compatibility as much as possible at this point.

Regarding some of the points you bring up and try to address, i agree and don't agree. The amount of custom code isn't as great as I think you perceived. Since it was stored as meta data on the aggregated posts, showing things would be as simple as echo get_post_meta( ... ); and with the new setup, the_post_thumbnail(); I will agree that with many themes coming with the_post_thumbnail() already ready and in place, it's merely less needing done immediately..

@brocheafoin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2017

I agree that not a lot of custom code is needed, but why require custom code at all when it's not necessary? Also, there is still the issue of the image sizes themselves, which may differ between the aggregated and the aggregating blogs. Agree to disagree? ;-)

I'll add back the current thumbnail meta fields to the aggregated posts. If I do this, will you consider merging this?

@@ -79,9 +80,11 @@ function hooks() {
add_action( 'save_post', array( $this, 'do_post_sync' ), 10, 2 );
add_action( 'wds_multisite_aggregate_post_sync', array( $this, 'save_meta_fields' ), 10, 3 );
add_action( 'wp_update_comment_count', array( $this, 'do_comment_sync' ) );
add_action( 'updated_post_meta', array( $this, 'update_thumbnail_meta' ), 10, 4 );

This comment has been minimized.

Copy link
@tw2113

tw2113 Jul 13, 2017

Member

Matching indent please.

if ( $this->options->get( 'tags_blog_thumbs' ) && ( $thumb_id = get_post_thumbnail_id( $post->ID ) ) ) {
// Post thumbnail
if ( $this->options->get( 'tags_blog_thumbs' ) && ( $thumb_id = get_post_thumbnail_id( $post->ID ) ) ) {
$this->meta_to_sync['thumbnail_url'] = wp_get_attachment_url( $thumb_id );

This comment has been minimized.

Copy link
@tw2113

tw2113 Jul 13, 2017

Member

Matching indent please.

}
// custom taxonomies
// custom taxonomies

This comment has been minimized.

Copy link
@tw2113

tw2113 Jul 13, 2017

Member

Matching indent please.

if ( $this->options->get( 'tags_blog_thumbs' ) && !empty( $meta_to_sync[ 'thumbnail_url' ] ) ) {
$this->set_thumbnail_by_url( $meta_to_sync[ 'thumbnail_url' ], $post_id );
unset( $meta_to_sync[ 'thumbnail_url' ]);
}

This comment has been minimized.

Copy link
@tw2113

tw2113 Jul 13, 2017

Member

Matching indent please for the trailing }

@tw2113

This comment has been minimized.

Copy link
Member

commented Jul 13, 2017

Gave some quick nitpick comments for some minor details.

@mmcachran when you get a chance, can you look over this PR and see if anything stands out for you?

Other than that, I have no present problem with merging it in. Would love for some of the people commenting in the issue to give it a test run as well.

brocheafoin added some commits Jul 14, 2017

Preparation for merge
* Code style cleanup (tabs were spaces)
* Return of the current thumbnail meta fields added to the aggregated
post
@brocheafoin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2017

  • Fixed the indentation issues (IDE was configured for spaces and not tabs).
  • Added back the meta fields currently used for post thumbnails

One or two methods were ripped from Automattic's aggregator, so contributors list should be modified accordingly.

The methods I copied and modified were all written by @philipjohn, if I'm to believe the blame.

@tw2113

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

Still wanting someone besides just me to do a quick code review this before we merge in, for what it's worth.

@10phillyphan

This comment has been minimized.

Copy link

commented Dec 18, 2017

Hey, I don't have much coding experience so I wouldn't be able to contribute much at all, but I was wondering if you (@brocheafoin ) could add support for scheduled posts. Your code for the plugin file works perfectly when pushing publish on a multisite, but when it is schedule for, say, 10am, the thumbnail will not transfer to the main blog. It works perfectly when straight-out pushing "Publish", but just not when scheduling.

Any chance you can help out with that?

@brocheafoin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2017

Hi @10phillyphan, I wouldn't mind taking a look at it, but I'm also waiting for this pull request to be merged into the main branch. @tw2113, you were looking for more feedback; @10phillyphan here says they've been using this branch successfully. Does that count as enough feedback? Thanks!

@10phillyphan

This comment has been minimized.

Copy link

commented Dec 31, 2017

@brocheafoin awesome, I appreciate that! Yes, it definitely works for me. No other plugin does exactly what I need it to do, but this one is as close to perfect as it can be. The scheduled post not showing the thumbnail issue is the only thing I still need to be fixed.

@tw2113

This comment has been minimized.

Copy link
Member

commented Jan 1, 2018

For what it's worth, you should be able to pull theirs into yours, without us merging anything first. They would just want to do the PR against yours. Perhaps we could get both merged in at once instead of multiple PRs.

just some thoughts.

@bibiwan

bibiwan approved these changes Jan 9, 2018

Copy link

left a comment

it's working :)

@tw2113 tw2113 merged commit 98b8413 into WebDevStudios:master Jan 10, 2018

@streiten

This comment has been minimized.

Copy link

commented Jun 4, 2018

was just done fixing #8 with pr #18 when i saw the progress here nice to see this advance!

@tw2113

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

@streiten is your PR still needed then? or did it end up getting covered here?

@streiten

This comment has been minimized.

Copy link

commented Jun 4, 2018

for legacy support i guess. when testing the new plugin version with old posts it wasnt covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.