Skip to content

Conversation

@salcode
Copy link
Contributor

@salcode salcode commented Oct 31, 2013

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

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.
@GaryJones
Copy link
Collaborator

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
Copy link
Contributor Author

salcode commented Oct 31, 2013

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
Copy link
Owner

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
Copy link
Collaborator

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
Copy link
Owner

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.

Replaced do_shortcode() with $GLOBALS['wp_embed']->autoembed() 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.

This is an improvement over my previous pull request, which used apply_filters( 'the_content' and therefore applied signficantly more filters than necessary.  Thank you to GaryJones for this improvement.
@salcode
Copy link
Contributor Author

salcode commented Nov 1, 2013

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.

…ress

Replace $rawattr with $attrs in method rebuild_shortcode in
class GistPress as per @bradyvercher's comment in
[Issue 34](#34)

Old Erroneous Behavior: [gist 7243951 1]
New Corrected Behavior: [gist id="7243951"]
@bradyvercher bradyvercher merged commit 8f8cd16 into bradyvercher:develop Nov 1, 2013
@bradyvercher
Copy link
Owner

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
Copy link
Collaborator

@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
Copy link
Contributor Author

salcode commented Nov 1, 2013

@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
Copy link
Collaborator

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.

@GaryJones GaryJones added the bug label Apr 20, 2014
@GaryJones GaryJones added this to the 2.0.2 milestone Apr 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants