WordPoints / wordpoints

Points plugin for WordPress
GNU General Public License v2.0
20 stars 15 forks source link

Autoload all classes #372

Closed JDGrimes closed 7 years ago

JDGrimes commented 8 years ago

After the Hooks API was merged in #321, we now have a built-in autoloader. We should move all of our classes so that they can take advantage of this. This is a potentially breaking change, though it doesn't have to be, so we need to decide exactly how we want to do it. Probably not in 2.1.0.

While merging the code I happened to think that we might as well put our admin classes in with the other classes in a single directory, since they'll be loaded only as needed. However, then I happened to think that our fallback will always load them them then if autoloading is disabled, so we might at least want to wait to do this until WordPress bumps its version requirement to 5.3. Also, our directory structure relating to the admin is currently such that everything for the admin is grouped together: classes, functions, assets, etc. Unless we change that, it would make sense to keep the admin classes with the other admin code, despite the fact that this adds an extra autoload directory.

JDGrimes commented 8 years ago

On a tangent, note that as of WP@37636 (4.6) WordPress provides its own autoloading shim when SPL is disabled. So in theory we could remove our fallback once we update the required version to 4.6. However, it should be noted that the shim requires that nothing else be declaring the __autoload() function, and so is not guaranteed to work. So we might want to wait until WordPress core's operation actually relies on that shim (or the PHP version requirement is upped) to remove the fallback feature of our autoloader.

JDGrimes commented 8 years ago

However, then I happened to think that our fallback will always load them them then if autoloading is disabled

Though we really don't have to, we could possibly come up with a safe way to detect the admin classes and only load them if is_admin(). But that is really separate from this ticket.

JDGrimes commented 8 years ago

One potential issue with this is that the classes won't have been named with this in mind, and so they might mess up our carefully planned directory structure in the autoload directories. But I guess we'll just have to take a look and see how bad it would really be.

JDGrimes commented 8 years ago

Also, our directory structure relating to the admin is currently such that everything for the admin is grouped together: classes, functions, assets, etc. Unless we change that, it would make sense to keep the admin classes with the other admin code, despite the fact that this adds an extra autoload directory.

One possible plus of this would be we could maybe flatten our includes tree, which would be a slight performance boost, by eliminating the admin.php files and placing those includes directly in the main file for the plugin/component. Although we could do that anyway.

JDGrimes commented 8 years ago

Ideally, we should get these classes autoloaded as soon as possible. Reason being, that not autoloading them causes any module classes that extend them and that are autoloaded to produce a fatal error when the autoload file is being validated.

Running "autoloader:all" (autoloader) task
>> Error testing autoload file src/classes/index.php:
>> PHP Fatal error:  Class 'WordPoints_Points_Shortcode' not found in /Users/johngrimes/git/_wordpoints-modules/sponsor-clubs/src/classes/shortcode/sponsor/link.php on line 16
>> PHP Stack trace:
>> PHP   1. {main}() /Users/johngrimes/git/_wordpoints-modules/sponsor-clubs/dev-lib/bin/verify-php-autoloader:0
>> PHP   2. simulate_autoload_fallback() /Users/johngrimes/git/_wordpoints-modules/sponsor-clubs/dev-lib/bin/verify-php-autoloader:48
>> PHP   3. require_once() /Users/johngrimes/git/_wordpoints-modules/sponsor-clubs/dev-lib/bin/verify-php-autoloader:26
Warning: Task "autoloader:all" failed. Use --force to continue.

Aborted due to warnings.

Process finished with exit code 3
JDGrimes commented 8 years ago

Note that we don't really have to move the files at all, now that the autoloader is purely a classmap. We could just add those classes to the map inside of our Grunt config file, and we'd be good to go. The only question would then be whether we want to deprecate those files eventually, and how to proceed in regard to that. But really, we don't have to decide that at the same time we begin autoloading those classes, it can wait.

JDGrimes commented 8 years ago

It might be more of a hack to the Grunt task than I originally thought though, to not move the files, since the directory structure gets in the way. But that doesn't mean that we still couldn't do it without moving the files. We just wouldn't have Grunt generate the classmaps for us, we'd manually add them to each directory that there are classes in that need to be autoloaded.

JDGrimes commented 8 years ago

We've got a list of classes that aren't in their own files yet in https://github.com/WordPoints/wordpoints/issues/498.

I think this should be our plan:

  1. In 2.2.0, add classmaps to load those classes that can be autoloaded.
  2. In 2.3.0 we can then try to move those classes that can't be autoloaded to their own files.
  3. Then we can also see about moving the ones that are being autoloaded to the autoload directory and deprecate the old files.

I'm not tackling the latter two right now because we don't have time to do so before we beta (which I'd like to do today).

JDGrimes commented 8 years ago

So, we've added the classmaps, we'll pick up there and tackle 2 and 3 above in 2.3.0.

JDGrimes commented 7 years ago

We are now autoloading all classes, and have moved some of them. Actually, we've moved all of the generic core classes (oh, except the un/installers, see #617), but the components' classes are where things get sticky in regard to naming conventions. I think that we can probably move some of them, though.

JDGrimes commented 7 years ago

Beside the un/installers, one other class not being autoloaded is the rank admin Ajax class. Currently it is designed to be loaded basically only when there is an Ajax request, but we'll probably be deprecating it in favor of a REST API endpoint eventually anyway. So for now, I don't mind leaving it.

JDGrimes commented 7 years ago

As far as the main purpose of this ticket though, I think that we're done here. Let's open a new ticket for moving the rest of the classes to the classes directories.