aaroneaton / better-yourls

Source code for the Better YOURLS WordPress plugin
21 stars 6 forks source link

Short urls not being (correctly/timely) generated for scheduled posts #15

Open stayallive opened 8 years ago

stayallive commented 8 years ago

Hey Chris, it looks like this is still not fixed even after the latest update (and the discussion in #13). We are noticing ?id=123456 urls again used by our Twitter plugin for scheduled posts :(

jphorn commented 8 years ago

Example: https://twitter.com/iCulture/status/686199857682477060 (I'm the site owner)

We use WP to Twitter (Pro) for sending our articles to Twitter. This plugin is configured to use WP's internal shortlink mechanism.

2016-01-11_14_13_33

2016-01-11 14 15 16

This works perfectly for all posts and post types, except for articles which have been published on schedule.

ChrisWiegman commented 8 years ago

Which twitter plugin are you using? I'm not seeing the same with mine.

jphorn commented 8 years ago

CW: Which twitter plugin are you using? I'm not seeing the same with mine.

JPH: We use WP to Twitter (Pro) for sending our articles to Twitter.

;-)

ChrisWiegman commented 8 years ago

Thanks. I'll try to see if I can play with a free version of it to see where it is getting it's link.

jphorn commented 8 years ago

This should be the plugin in svn. I believe the relevant part is in wp-to-twitter-shorteners.php. The author is @joedolson.

ChrisWiegman commented 8 years ago

Even better. Thank you!

joedolson commented 8 years ago

If there are any issues that you'd like my comments on, please let me know!

ChrisWiegman commented 8 years ago

@jphorn I've tried this with just Better YOURLs and WP to Twitter and it is working just fine for me. I know this probably seems silly but can you please confirm YOURLs is configured correctly?

@joedolson I think this should work here, at least it is in my testing. In exchange for a sanity check I would gladly offer a PR or 3 in return.

ChrisWiegman commented 8 years ago

@Stayallive Which Twitter plugin are you using?

joedolson commented 8 years ago

Just tested, and it worked just fine for me, as well. Steps: Installed and enabled Better YOURLS, added my YOURLS install info. Set WP to Twitter to use WordPress as it's URL shortener. Published a post. URL returned was the YOURLS short URL as expected.

ChrisWiegman commented 8 years ago

Thanks @joedolson ! I'll definitely return the favor here shortly.

@jphorn Can you please confirm?

stayallive commented 8 years ago

@ChrisWiegman I work for iCulture so @jphorn an my problems are the same :)

Did you test by scheduling a post, since directly publishing works just fine?

jphorn commented 8 years ago

@joedolson It works fine for directly published posts for us as well. But not for scheduled posts (where WordPress is set as the short link generator in WP to Twitter)

joedolson commented 8 years ago

Yes, just realizing that - just a sec. Allowing scheduled post to trigger.

joedolson commented 8 years ago

Yes, confirmed - that didn't work. I'll look into it.

ChrisWiegman commented 8 years ago

Oh. I'm sorry. I gotcha now. About to head out but I'll test shortly.

joedolson commented 8 years ago

I just took a look at the code, and at least on superficial glance it seems like everything should be fine. WP to Twitter does everything on save_post at priority 15; Better YOURLS is processing on transition_post_status at priority 10. The transition post status hook runs before save_post, so this really should always happen before WP to Twitter.

There are some cases where WP to Twitter would run on future_to_publish, but when it does that it runs at a slightly later priority (16), which should still happen after Better YOURLS.

Not sure what's off, here...

ChrisWiegman commented 8 years ago

I've tested this as well with a few scheduled posts over the last few hours and all worked as expected. I can't help but wonder if perhaps something is conflicting here.

jphorn commented 8 years ago

Let me start by saying we appreciate the testing efforts very much. We'll take another look in our setup. The only other conflicting plugin I can remember from the top of my head right now is Mashshare, our sharing plugin.

ChrisWiegman commented 8 years ago

@jphorn I'll take a look at it with that as well if it's a free plugin. Would love to find some way I can improve this if possible

stayallive commented 8 years ago

Hey Guys, I have just set-up a totally clean WordPress and installed Better YOURLS and WP to Twitter from the plugin repository and created a dummy Twitter account to test this myself too, however... it breaks :( See: https://twitter.com/stayallive_test

If you guys want to take a look at that setup send me a email (listed on my GH profile)

ChrisWiegman commented 8 years ago

Thanks @Stayallive I'll give it another shot this week. I'm wondering if my install was perhaps running CRON weird.

jphorn commented 8 years ago

Please note: don't configure WP to Twitter with the YOURLS setting, but use 'WordPress' as seen in the image is this comment. We need to have it set to 'WordPress' to take advantage of Mashshare's ShortURL add-on.

jphorn commented 8 years ago

@ChrisWiegman Did you have a change to look into this again?

ChrisWiegman commented 8 years ago

I'm afraid I haven't had a chance to circle back. It's still on my radar and after a few major projects finish up this weekend I'm hoping I can make this next

jphorn commented 8 years ago

Thanks, appreciated. Just wanted to check in :-)

ChrisWiegman commented 8 years ago

I've finally (sorry about the delay) gotten to look back at this. Removing any missed post scheduled does indeed recreate the error however, WP to Twitter isn't even using the proper permalink (it's sharing the ?p=post_id version). As short links are based on permalinks this actually makes sense from Better YOURLs point of view.

@joedolson Could this be as I'm trying this with the free version? Would love to help dig deeper into the issue but don't want to waste my time if the code base here makes a difference.

joedolson commented 8 years ago

No, that shouldn't make any difference at all. When WP to Twitter operates with WordPress set as the shortener, it uses wp_get_shortlink() to fetch the URL, if that helps.

ChrisWiegman commented 8 years ago

So after further investigation it appears wp-to-twitter isn't using the permalink at all in scheduled posts. This isn't the case in normally published posts. You can see this by looking at the tweets of each. As Better YOURLs will defer to the intended permalink it doesn't find anything to shorten in this case. I'm afraid this will probably necessitate opening an issue with wp-to-twitter.

joedolson commented 8 years ago

Based on this comment - what happens if the "no shortener" option is selected in WP to Twitter? When WordPress is selected as shortener, then WP to Twitter will use wp_get_shortlink(); but if you want to use the permalink, then that's what's passed if WP to Twitter doesn't use a shortener. The path by which it's passed is convoluted, but that's what you ultimately get.