search

hyunsupul / aesop-core

Open-sourced suite of components that empower interactive storytelling in WordPress.
http://aesopstoryengine.com
GNU General Public License v2.0
244 stars 56 forks source link

WordPress VIP Issue #2 #222

Open bearded-avenger opened 9 years ago

bearded-avenger commented 9 years ago

Before we send this for approval there are a few things we need to take care of:

Plugin registration hooks are prohibited. This alone will now require a completely different codeset for VIP. We need to utilize these hooks for the general user.

https://github.com/bearded-avenger/aesop-core/blob/master/aesop-core.php#L48 https://vip.wordpress.com/documentation/code-review-what-we-look-for/#plugin-registration-hooks

bearded-avenger commented 9 years ago

I'm not sure what the best approach is to this. I'd like to avoid having to maintain two different sets of largely the same code. 95% of me is pretty sure these activation hooks have to stay, as it's considered best practice for a publicly distributed plugin. i.e, it's not something we can remove.

michaelbeil commented 9 years ago

This does present quite the predicament. Could we utilize some if/else statements to differentiate activation within VIP ~ wpcom_vip_load_plugin() and .org?

bearded-avenger commented 9 years ago

could possibly use wpcom_is_vip

michaelbeil commented 9 years ago

could work.

harvitronix commented 9 years ago

I'm probably missing something with the activation, but from what I can tell, the activate function doesn't do anything, and the deactivation only deletes an option. If this is correct, then this should be safe as VIP will merely ignore these functions since they'll never be fired.

Not the cleanest solution, but is that at least a logical place to approach this from?

harvitronix commented 9 years ago

Also, I don't think you can use wpcom_is_vip as that's a helper function that most blogs won't have available. Though I suppose you could wrap it in a check for that function existing first...

bearded-avenger commented 9 years ago

Yeah it'd be wrapped in a function check, and is a possible solution to not having to maintain two different codebases. I'm still looking for the right documentation to see if that's even the right function to call to check if the plugin is on VIP or not.

When the plugin is activated, it basically loads the public and admin classes off activation hooks. So, what I've done is just created another branch for VIP.

https://github.com/bearded-avenger/aesop-core/tree/vip

michaelbeil commented 9 years ago

I think the implementation Nick has could do it, albeit there are two separate codebases.

bearded-avenger commented 9 years ago

hah, removed all teh backwards compatibility and I get rewarded? That's not right! ;)

image 2015-03-17 at 3 20 22 pm

michaelbeil commented 9 years ago

there we go.

bearded-avenger commented 9 years ago

@harvitronix I think this gets us pretty close

harvitronix commented 9 years ago

@bearded-avenger I did a quick scan the other day to find unescaped output and found some. They're fixed (but not QA'd) in #231.

I will try to do a QA round tomorrow, unless you guys have regression tests that you can use to QA it more quickly.

michaelbeil commented 9 years ago

That'd be great if you could.