Create a base updater class

[GaryJones]

There's a couple of methods that both the updater classes use, so to reduce duplicate code, it would make sense to have a base updater class that the plugin and theme classes both extend.

[jazzsequence]

+1 I was thinking this, too, the other day, when I was looking at the code.

[GaryJones]

I think it's worth waiting for Andy to make a decision on the current PRs before starting on this.

[jazzsequence]

Definitely. Just adding my support :)

[afragen]

I should have time today to review the PRs. Initially I combined 2 different classes, well I converted the theme code to a class, for the plugin. Much of have code just seemed different enough or variables would change to be more abstract than related to plugins or themes when creating a base class.

The other issue is if we do this we lock users in to using the entire code base and not just being able to use the plugin or theme class for their project.

Thoughts?

[jazzsequence]

The other issue is if we do this we lock users in to using the entire code base and not just being able to use the plugin or theme class for their project.

IMO, if that's what they wanted to do, they wouldn't be using this plugin, they'd be using the theme-specific or plugin-specific class in their own code.

[afragen]

But do we want to lock them out of using our theme-specific or plugin-specific code?

[afragen]

In their code

[jazzsequence]

I don't think that what @GaryJones is suggesting is necessarily contrary to having theme- or plugin-specific code. It's just a matter of abstracting the stuff that is similar in both classes, creating a base class out of that, and then the plugin updater class will extend that, adding all the necessary stuff for the plugin side, and the theme updater class will extend the base class and add all the necessary stuff for the theme side. So instead of two, really similar classes, you'd have one class that's the base, and two additional classes that build off of the base.

That would be more extensible for theme- and plugin developers, rather than less.

[afragen]

I actually think this is a very good idea. I'm just playing devils advocate :wink:

[GaryJones]

@jazzsequence is right with his last comment. Think of it as reducing duplicate code. We wouldn't be locking anyone out of anything. If they wanted to make use of it in their own plugin, they'd just need to include the base class file as well as the plugin (or theme) class file as well.

The refactor also gives an opportunity to standardise some of the internals as well, so that the classes more generally match each other, even when they have their own methods for doing things.

[afragen]

PRs are all merged. Looks like we can start on this.

[afragen]

I've made a bunch of commits that should go a long way towards making the renaming code easier to refactor.

[afragen]

OK, 1.5.0, pushed, tagged and released. I think we should work on this next as it will likely effect all other efforts.

[pdclark]

Are you close on this, would you like input on how to create multiple interfaces from a parent class, or do you think you'll be keeping it Github only? I have working code for this.

[GaryJones]

I think to start with, we want an interface for multiple implementations, even if GitHub is the only initial implementation. Input welcome :-)

[afragen]

@pdclark - I've been thinking (not coding) about this. I envision having a class for each Git repository and loading each particular class from the base class based upon which extra headers are in the plugin.

eg. GitHub Plugin URI: would load class-github-plugin-updater.php BitBucket Plugin URI: would load class-bitbucket-plugin-updater.php, etc.

I think modularizing it this way makes development and updating much simpler than a single class-git-plugin-updater.php that does everything.

In answer to your question above, I'll accept and want any help I can get. As @GaryJones can tell you, this isn't my day job. :wink:

[pdclark]

Glad to hear it! It's not my day job either, but I got re-obsessed with Git plugins again this week. ;)

[afragen]

This project was rejected for the WP.org repo. :frowning: But I wanted it for personal use so... @GaryJones has really helped me take it the next level.

FYI, the avatar is me at the day job. :wink:

I'll have to read up a bunch to figure out the base class thing but I think, going forward, it makes a lot of sense.

[pdclark]

Awesome. :) I living in Anchorage right now because my wife is in her Family Medicine residency here. We're on year 2! My best man in our wedding is in residency for Anesthesiology, and our pre-marital counselor was a Pediatric Odolarangologist. We have ...lots... of doctors in our lives!

I know being a trauma surgeon is intense and demanding! I'm impressed you're maintaining this and many other scripts on top of it! (Not to mention being a dad!)

I work on WordPress and WordPress sites full time.

Base classes aren't too much to worry about — if you've ever created a WordPress Widget, you've already been using them! You can think of them layering pieces of acetate on top of each other: the parent defines methods (functions) shared by all it's children. The children can then use what was created, or override the parent by laying a new function of the same name "on top" of it.

There are a few other data structures that might be new if you see them in my plugin:

__get() method

This is a "magic method" in PHP. Defining it, then setting all the object vars to protected makes it so that when I access things like $plugin->remote_version, it sees that the var is protected, then returns the value of $plugin->get_remote_version() instead.

Plugins as objects

When I create the array of plugins, I load each plugin as an updater object (not just an array of information about a plugin).

This allows each $plugin in GPU_Controller::$plugins to be a self-managing piece of logic. GPU_Controller asks each plugin the same questions, like "what is your remote_version?", and each plugin can run whatever logic is appropriate to answer the question correctly.

This is sometimes referred to as the Strategy Design Pattern.

Singleton Design Pattern

GPU_Controller is set up as a singleton. This just ensures two things: There's only ever one version of the plugin running, and it can be accessed anywhere with GPU_Controller::get_instance().

This is considered a best practice because it avoids all scope issues (inability access the plugin, or global variables conflicting with other plugins).

Delegation of roles

This isn't really an advanced thing, but I thought it might be helpful to see how different jobs are split up in the plugin:

plugin.php

Just make sure loading everything else isn't going to crash anyone's site.

GPU_Controller

The main plugin "wrapper". Things like settings up hooks, managing options, loading templates, and setting up other classes go here. This is a loose Model-View-Controller design pattern. The controller's job is to connect the data models (plugin updaters, database settings) with the views (WordPress admin).

GPU_Controller::$admin

Instance of GPU_Admin. When there's a setting page or other admin GUI, it'll go in this class.

GPU_Controller::$plugins

An array of objects that are instances of GPU_Updater_Github or GPU_Updater_Bitbucket or GPU_Updater_Gitweb.

GPU_Updater strictly defines that the sub-classes MUST respond to some specific minimum methods.

That's what the lines like abstract protected function api( $url ); do. It's saying "any child class that wants to use GPU_Updater as a parent absolutely must define a method called api that takes the argument $plugin.

[pdclark]

PS: Can you elaborate why the plugin was rejected from .org? I imagine it might have been under the clause below, but then, upgrades are mentioned as okay (but... they might mean upgrades from wp.org).

No sending executable code via third-party systems. Use of third-party systems is acceptable in a service-client type of model, but sending actual PHP or other executable code over the network is considered a security risk. Executing outside code like this is not allowed except for specific and very carefully considered cases (such as during upgrades, for example).

Was it rejected because of this, or another guideline?

Do you know who reviewed your plugin submission? (Pippin? Mika? Otto?)

[afragen]

It was rejected from .org based upon the guideline you cited. Actually, the review process was very interesting. Pippin sent me the rejection and I sent back asking some other questions and pointing out that, in fact, there were other plugins in the repo that did this. Mika wrote back saying there was nothing inherently wrong with the plugin but that there was a long discussion about it and it was felt that they didn't want to promote outside repos.

I don't have a problem with the rejection. It initially surprised me as my other plugins have been accepted. But I understand about not wanting to promote something else. I never heard from Otto directly but judging from his comment, http://www.wptavern.com/how-to-install-wordpress-plugins-directly-from-github#comment-48234

All in all I exchanged a number of emails asking questions and I found @pippinsplugins and @Ipstenu to be very fair and helpful.

[Ipstenu]

While the plugin itself was NOT actually violating the rule (it was just letting you use another source), but it was encouraging others to do so. I like the code, but it's just contrary to the ethos of the repository.

BTW, as I mentioned at the time, we did pull the other plugins you told us about :)

[afragen]

@Ipstenu I was intending to put in that part, I just forgot. And I understand completely about repository plugins not encouraging automatic updating of non-peer reviewed code. And for the record, you, Pippin and Otto do a fabulous job in maintaining the repo.

[afragen]

Here's my initial go at it, https://github.com/afragen/github-updater/commit/f2390bd66249a2019459bb913c79f93574e01e3e

[afragen]

Check out the develop branch, I'm getting much closer to this. The only issue I'm having is passing an instance of a class object as a variable to another class. I keep getting an undefined variable error. You won't see this in the code as I haven't put this part up. If you look at https://github.com/afragen/github-updater/commit/c95e82e3f04cb259da99db050a8d75bbec9fe4ba you'll get an idea.

[afragen]

WOOT! The refactoring is completed. Pushed and tagged.