eventespresso / event-espresso-core

Event Espresso 4 Core for WordPress: Build an Event Ticketing Website Today!
https://eventespresso.com
GNU General Public License v3.0
121 stars 87 forks source link

IMP: Hooking directly into the WP Upgrade System #187

Closed thebeline closed 7 years ago

thebeline commented 7 years ago

I was interested in what it would take to hook directly into the WP upgrade system, and it looks like the filter site_transient_update_plugins would give you a list of plugins, which you could then modify the array of each's update and update_version and package.

I am having a hard time determining how you currently handle upgrades, but I am suggesting this method (or, hooks, rather) because this will seamlessly integrate with the WP Cli plugin, and, depending on how it is currently done (again, I have write permissions locked down, so I can't perform upgrades from with web anyway) it will also integrate with the core WP Upgrader.

Relevant WordPress Classes:

Hope this helps... If the current update tests and hooks were a little clearer, I would make the mods myself, but I can not seem to decipher how add-ons are upgraded, so I will leave it to the pros. ;-)

nerrad commented 7 years ago

Can you please outline what problem you are wanting solved by this issue? EE does hook into the WordPress update system. Event Espresso Decaf has its updates served from WordPress.org. In the full (we call caffeinated) version of EE4 updates are done via our own servers but utilizing the WordPress update system.

thebeline commented 7 years ago

Again, I am not sure how it is hooked into the existing WP Update system, but it is not hooked in what I would call "entirety".

The Updates Available flag seems to show up on the web end, however, it does not show up when using WP Cli, which uses the transient directly. Therefor plugins do not register as ready to be updated via the command-line, which is the most secure way to update as it allows strict directory permissions.

Regardless of how I am updating, it seems like hooking directly into the transients handler to trigger updates would be the best way. This way you could still do your automatic updates by querying this handler and updating only your add-ons, and the user can do it if they have strict directory permissions.

thebeline commented 7 years ago

Example:

On Web:

There is a new version of Event Espresso - Barcode Scanner (EE 4.6.0+) available. View version 1.0.10.p details or update now.

I can not Update Now as my web-server does not have write permissions on the plugins directory.

From WP Cli:

$ wp plugin list
+-----------------------------+----------+--------+----------+
| name                        | status   | update | version  |
+-----------------------------+----------+--------+----------+
| eea-barcode-scanner         | active   | none   | 1.0.9.p  |
+-----------------------------+----------+--------+----------+
nerrad commented 7 years ago

Our updates are controlled in /core/third_party_libs/pue/pue-client.php. In there you'll see that site_transient_update_plugins IS hooked into. We do not (currently) test EE against command line tools such as wp-cli. It is on my radar to provide more tight integration in the future (I use wp-cli for a number of things as well) but its not a high priority atm.

For EventSmart we do our deploying via a git setup that works really well.

thebeline commented 7 years ago

WP Cli does nothing special, starting here, the path of execution ends up at get_update_info which gets the plugins and their statuses from get_site_transient('update_plugins'). If the update statuses are not present there (which I can confirm, they are not), then something is missing.

Perhaps it is a bug in pue-client.php.

nerrad commented 7 years ago

Or perhaps its simply that wp-cli grabs the site-transient before pue-client.php has a chance to add its filter because of a timing issue. I want to re-emphasize a point I've already made:

We do not (currently) test EE against command line tools such as wp-cli.

That means, we're not actively using wp-cli to see if it works well with EE. So although I appreciate it may be something that affects your workflow, the majority of our user base doesn't even have a clue what wp-cli is let alone how to use it, so unfortunately its not much of a priority to ensure EE plays nice with wp-cli atm.

Since this clearly affects your workflow, I encourage you to see if indeed it is what I suspect it is in this case which is when wp-cli calls get_site_transient('update_plugins') the filter callback pue-client.php is not even invoked (which likely indicates a timing issue for wp-cli code execution).

Have you had similar experience with any other commercial plugins that serve updates from their own servers when using wp-cli?

thebeline commented 7 years ago

The cause is the is_admin check here: https://github.com/eventespresso/event-espresso-core/blob/master/core/EE_System.core.php#L798

Bypassing this check causes it to work.

Changing the line to read:

if ((is_admin() || ( defined('WP_CLI') && WP_CLI ) ) && apply_filters('FHEE__EE_System__brew_espresso__load_pue', true)) {

Makes the whole shibang work. Just changed the line and updated the barcode add-on, all went well so far.

The only thought I would have is that there are 71 lines in EE that use is_admin() and I would not be surprised if one or more were related to conditional migration behavior. It works well for a code-only update, but I would be interested to see how it would handle an update that had migration hooks. I will have to check at some point.

nerrad commented 7 years ago

Good find! Ya so again, because we are NOT explicitly testing against WP_CLI, it is likely you will find other issues that affect your workflow. Feel free to document those issues as you find them and we'll consider updating the codebase to accommodate WP_CLI usage a bit better.

thebeline commented 7 years ago

Sidebar: I am not trying to be difficult, I am a web software engineer as well, so i get that I may be coming off as the kind of customer who thinks they know more than they do, and likes to tinker, which I am not (well, I am, but I actually do know quite a bit, and I am not blindly stabbing). I am sorry if I am causing a stir. The unfortunate truth is, I am going to be contributing a lot more Issues/PRs than most any of your clients. Please accept my apologies ahead of time.

nerrad commented 7 years ago

One thing about migrations. Although we do hook into the WP activation hooks, we don't rely on them. Primarily, because bulk updates done through the WP interface do not fire the activation hooks.

So, we have fallbacks in place that detect version changes on a normal admin request and run any necessary migration process. So my guess is that you shouldn't run into any problems if you update via wp-cli, the first time you visit your admin the system should detect any version change and act accordingly.

WRT your sidebar. Thanks for the clarification. Appreciate the feedback you have been giving, it's pretty clear from what you've posted so far that you have some idea of how to get around code.

One thing you might want to be aware of is that we are actively working on improving the structure and are transitioning to implementing PSR standards (as opposed to WP code styles) across our codebase.

thebeline commented 7 years ago

Side question: in EE_Event_Editor_Decaf_Tips and EE_Event_Editor_Tips, is there a particular reason you are implicitly defining the array indexes? You do some fun skipping around in EE_Event_Editor_Decaf_Tips, but EE_Event_Editor_Tips is all sequential (but still implicitly defined).

If there is a method to the process, awesome, let me know, because I have a few to add (you are going to get sick of this real fast... :-P).

thebeline commented 7 years ago

On your sidebar-sidebar: Awesome! Having things namespaced out nicely would help a bunch, as would having auto-loading hooked in (bit me a few times so far). I remember migrating our two (larger) products to namespacing... Sure did upset a bunch of people out of the gate, but I noticed my shoulder got tapped on a lot less after they got used to it.. :-D

nerrad commented 7 years ago

Side question: in EE_Event_Editor_Decaf_Tips and EE_Event_Editor_Tips, is there a particular reason you are implicitly defining the array indexes? You do some fun skipping around in EE_Event_Editor_Decaf_Tips, but EE_Event_Editor_Tips is all sequential (but still implicitly defined).

It's super helpful (even for smaller questions like this) to create a new issue. It's more likely to get attention and less likely to get buried.

thebeline commented 7 years ago

All concerns addressed, behavior identified, PR submitted. Closing.