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

Store unprocessed HTML in a transient #1

Closed GaryJones closed 11 years ago

GaryJones commented 11 years ago

Take an example where you have a WP Post, and in it you display a single file from a single Gist, and then break that code down to show certain lines as you explain it.

As things stand, the processed HTML is stored under an md5 hash of all of the shortcode attributes. However, showing the complete code, then 5 portions of it means currently making 6 JSON calls.

Consider making a JSON call and storing the unprocessed HTML in a transient with a key hashed from just the id and file (may be empty) attributes, and then processing the HTML (showing certain lines, highlighting certain lines, removing meta, removing line numbers etc.) and storing that in transients with a key hashed from all shortcode attributes, as per now? Before running off to Github for each snippet, check if the unprocessed HTML is already retrievable from a transient.

Post updates will have an extra lot of transients / post meta to delete but the benefit is potentially far fewer JSON calls when the post is viewed after the transients have naturally expired.

bradyvercher commented 11 years ago

It was originally working similar to this, but I removed the unprocessed transient and replaced it with the processed transient. This shouldn't be a problem to implement, I was just worried it might be getting a little out of hand storing the data in so many different places. I'll go ahead and take care of this after your pull request.

bradyvercher commented 11 years ago

Actually, I didn't have it working similar to that originally because updating the post was supposed to allow any changes to the Gist to be retrieved.

I went ahead and implemented the extra caching, though, and added a special attribute to force the remote request. Everything appears to be working correctly, but I'd appreciate a sanity check. And any recommendations for a better alternative to force the remote check are welcome as well.

GaryJones commented 11 years ago

Instead of doing a fetch attribute in the shortcode (which is only editable by the author, and they can do a post save / update to force a refresh anyway), how about letting it be refreshed from the client side via a querystring? e.g. example.com/code/make-foo-go-bar?gist=refresh or even just ?gistrefresh. ?

bradyvercher commented 11 years ago

Actually, re-reading your initial comment, it looks like you suggested deleting the raw transient on post save and re-saving it on the initial read so any subsequent requests to the same endpoint in the same post don't generate additional remote requests, which is a little different than what I implemented.

That would probably easier to manage, so I'll go that route, but I do I like the query string idea much better than the fetch attribute. Thanks!

GaryJones commented 11 years ago

you suggested deleting the raw transient on post save and re-saving it on the initial read so any subsequent requests to the same endpoint in the same post don't generate additional remote requests

Yes. While you could try and cache responses from the same endpoint from multiple posts, I think that would be edge case (two blog posts on the same site that show at least part of the same gist? Rare.), just doing it per post is fine - that covers a post like http://code.garyjones.co.uk/genesis-grid-loop/ which shows gists of individual methods, then one "put-it-altogether" gist, which currently results in 4 remote calls, whereas using your (very funky, btw) lines attribute would allow me to just reference different parts of the big gist which in turn should only require one call to Github.

bradyvercher commented 11 years ago

The transient is global and doesn't contain a reference to the post, it's just using the URL (plus optional file) to create the key, so if two posts are using the same Gist+file, then they'll use the same transient (not that it matters much after the initial request). In any case, I think it should function similar to the method you're describing.

Haha, not sure if funky should mean "pretty cool" or "somewhat ugly." The request actually came from someone else wanting to use it for tutorials and I thought it was a good idea for doing something like that. I might have to debug it a bit, though, it doesn't appear to be working right on my end.

GaryJones commented 11 years ago

Funky is a good thing in this case :-)

I was playing with it this morning, and it was working fine. Just merged upstream/dev and the line selection is still working correctly (although it looks like the stylesheet reference isn't.)

bradyvercher commented 11 years ago

The issue I was having was because of the cache, but I took care of it in c4c1e1b. In any case, I believe this is good to go.