bradyvercher / gistpress

WordPress plugin to add Gist oEmbed and shortcode support with caching.
GNU General Public License v2.0
143 stars 28 forks source link

Fix for oEmbed cache not clearing on update. #35

Closed salcode closed 11 years ago

salcode commented 11 years ago

Patch for issue #34

Replaced do_shortcode() with apply_filters( 'the_content' in method delete_gist_transients in class GistPress. do_shortcode() does not does not process oEmbeds, which prevented the shortcode method from being executed and clearing the cache.

Improved as per @GaryJones by using

do_shortcode( $GLOBALS['wp_embed']->autoembed( $post_before->post_content ) );
do_shortcode( $GLOBALS['wp_embed']->autoembed( $post_after->post_content ) );

Corrected $attrs instead of $rawattr as pointed out by @bradyvercher in comments on #34

GaryJones commented 11 years ago

Instead of applying the the_content filters (which might have many other filters that cause strange side-effects), could we not just do something like:

$GLOBALS['wp_embed']->autoembed( $post_before->post_content );
$GLOBALS['wp_embed']->autoembed( $post_after->post_content );

I'm not sure if that would work, but it would seem more direct than running through all of the_content filters when all we're trying to do is delete transients.

salcode commented 11 years ago

I agree with Gary that the_content filters seems like overkill. $GLOBALS['wp_embed']->autoembed() seems better.

I think the updated lines would be

do_shortcode( $GLOBALS['wp_embed']->autoembed( $post_before->post_content ) );
do_shortcode( $GLOBALS['wp_embed']->autoembed( $post_after->post_content ) );

I'll confirm and create a new pull request within the next 24 hours.

bradyvercher commented 11 years ago

I agree on not running the the_content filter if not necessary and actually gave the autoembed method a quick go last night, but didn't do the shortcode afterward which is probably why I didn't have any luck with it. The updated lines from @salcode look like they'll work.

GaryJones commented 11 years ago

Worth looking at the WP_Embed class itself as well. It filters the_content with the run_shortcode() method, and that unregisteres non-embed shortcodes, calls do_shortcode(), then reregisters the non-embed shortcodes - I guess that's so nested oEmbed shortcodes work fine or something? [column width=2"][gist ...][/column] might cause problems otherwise (this is a total guess).

At this stage, assuming @salcode's updated lines work, lets get that committed, a bug fix release potentially made live, then look afterwards at if it can be improved.

bradyvercher commented 11 years ago

I'll wait a bit for @salcode to update the PR. It's been lingering for a few months, so a few hours won't hurt, then we can push the fix live.

salcode commented 11 years ago

Thank you @bradyvercher and @GaryJones for GistPress, for allowing me to take part, and for being patient with my updated pull request. It has been a pleasure.

bradyvercher commented 11 years ago

Thank you @salcode for checking the plugin out and taking the time to find a fix a bug we'd missed.

Just a quick note: It looks like everything worked fine this time, but in the future, pull requests should be submitted against the develop branch so we're all working with the latest.

GaryJones commented 11 years ago

@bradyvercher If you go to the Settings page for this repo, then you can change the default branch to develop - all future PRs will then be to this branch by default.

salcode commented 11 years ago

@bradyvercher To clarify, are you suggesting my PRs should come from the develop branch instead of master?

I realize now my PRs were into bradyvercher:develop from salcode:master and I assume it would make more sense for them to come from salcode:develop. I will keep this in mind in the future. Thanks.

GaryJones commented 11 years ago

Yes, that's what he's saying. Otherwise, you can't make any more changes to your master branch until the PR has been committed, and that would leave you with no clean branch from which to do further hotfixes.

Have a read of http://nvie.com/posts/a-successful-git-branching-model/ - in this case, you would have created a branch from develop, called it something like feature/cache-clearing-bug, made your changes on that branch, then committed that locally, pushed to GitHub, and done a PR against @bradyvercher's develop branch (some of which you did anyway). If you look at #36 and #37 you'll see that I created specific branches for each improvement.