morrisonlevi/Ardent

Let Composer Handle Autoloading

mcordingley opened this issue · 9 comments

I see that the library currently implements its own autoload function. Why not just use PSR-4 autoloading and let Composer handle this? It'll make it easier to alter this library while it is on its way to 1.0 and will make it simpler for others to contribute.

If your concern is about performance, your current implementation is a class map. Running composer with the -o flag will generate a class map, instead of attempting to map namespaces to file system locations. Performance should be the same at that point and you'll have DRYer code, as you won't be repeating logic found elsewhere.

While composer is a helpful tool and many people do use it I think it is prudent to allow people to easily use this repository without composer. Notably the department at BYU I work for uses this without using composer. This is because our codebase is old and completely custom. Requiring composer for this single library would add an extra tool for no real gain.

The reason it doesn't follow PSR-4 standards is that there were too many files in the src/ folder. It was difficult to just look at the files on the filesystem and really have an understanding of what it is about. So I moved exceptions and iterators out of the main tree so it would be easier to understand the library at a glance.

Is there anything about this approach you'd like me to clarify or expand on? Anything you disagree with?

Ah. I assumed that by now everyone was using Composer. This is a long response, so I'm breaking it up with headers. It's your code and therefore your choice, and my apologies if I'm telling you things that you already know, but I'll err on the side of being informative but pedantic.

Composer

It is entirely possible for old code-bases to use Composer, even ones that have their own class loading systems in place. I've done it before. 👎 I'd argue that a project that doesn't do so is leaving too many useful tools out of reach, as quite a bit of code out there is Composer-only these days. The way an old code-base can be transitioned onto Composer goes as follows:

  • Code that is already loaded does not trigger an autoload from PHP. Your existing require and include statements should already work for things that are non-standard. This will handle files that contain multiple classes before you refactor them into their own, separate files.
  • In the event that multiple autoloaders are registered, they resolve in the order in which they were registered. So, you'd register your project-specific autoloader, followed by including vendor/autoload.php Your existing autoloads will still work and you can gradually migrate them into a PSR-compliant autoload scheme.
  • You can target the same folder with both a classmap and a PSR-2/4 autoload rule. More generally, the same area in the filesystem can be matched by multiple loading strategies. The first one able to do the load wins. This means that you can take code that hasn't been namespaced and convert it into the appropriate namespaces over time, refactoring files one by one.

Exceptions and Iterators Folders

PSR-2/4 standards don't preclude you from using sub-folders. They actively encourage it. Just rename the namespace on each iterator from Collections to Collections\Iterator and the exceptions to Collections\Exception. Then in the files that use them add use Collections\Iterator\Foo; You shouldn't need to otherwise alter your class files that consume these files. Because you rolled your own autoloader, you'll need to update the entries for these files to have 'Collections\Iterator\Foo' as the key, instead of 'Collections\Foo'.

More generally, it would help the readability of your classes if they had use statements at the top for all symbols that are not in the current namespace. Not only would the code inside of each class be more concise, but you'd be able to tell, at a glance, what dependencies a given class has on other types.

Right back at ya with "Is there anything about this approach you'd like me to clarify or expand on? Anything you disagree with?" :)

If I were to switch to composer's autoloaders I would still need to include src/function.php. I've never done this, but I suppose that using two different properties of "autoload" should work:

"autoload": {
    "files": ["src/function.php"],
    "psr-4": {"Collections\\": "src/"}
}

I'll try this later.

Yeah. That would do it.

I also think it possible to build a phar archive out of this that your legacy app could include. I still urge you to consider migrating your app over to using Composer, but I could send this as a pull request later on if you want.

I'm still not convinced on something: why is it better to have composer build the autoloaders? I already have autoloaders for those who don't want to use composer, but I also do support composer. Switching to composer to make the autoloaders means that my users now have to use composer. How is that a net win? They didn't gain any new functionality but now are required to use composer.

  • You no longer have to maintain your own autoloader. That shrinks your code-base, lowering your maintenance overhead and chance for bugs. Granted, the gains on both counts are small for a project like this, but it would definitely lower the amount of work whenever you add, remove, or simply move a class.
  • Contributers don't get surprised and have to go digging through the rest of the project code when they want to send you a PR. cough

...there's not actually much more that I can think of. Two questions for you, though.

  • Who doesn't use Composer these days? As in, do you anticipate anyone other than yourself not using it and why?
  • How do those users that you have, or anticipate having, use the code? Is it by copying the whole directory tree into their projects? By using the Phar builder that you have (and I just noticed) to build a phar and include it? If it's by way of a Phar, you can still set up for Composer and build the Phar off of that.

I admit that I missed the fact that it does support usage through Composer and that the major gains to be had by switching to Composer would only be for those working on this code, so the overall case for this is somewhat weaker than I had thought.

I would ask that you at least have your directory structure and namespaces match. That is a point of unwelcome surprise when using or reading the code and would be a prerequisite for if you should decide to go with Composer now or in the future.

Who doesn't use Composer these days? As in, do you anticipate anyone other than yourself not using it and why?

Most local groups and companies I know have been around long before the days of composer and haven't seen the value of switching.

How do those users that you have, or anticipate having, use the code?

We have a git submodule that we update occasionally. I am less certain of how other people use the code. I don't really have any way of knowing who just includes autoload.php, who uses the phar, and who uses composer's autoloader.

I would ask that you at least have your directory structure and namespaces match.

I have an experimental branch locally that has cut out some of the classes in the repository. In this branch the structure follows PSR-4.

  1. Amazing.
  2. Makes sense, though honestly just running composer update makes things much easier than using Git submodules. To each their own, I guess.
  3. Awesome.

@mcordingley I'm sure you've lost interest but I thought I'd let you know:

The current design of this library is going to be thrown out. I've tried alternative methods of designing these sorts of libraries in PHP and have liked them all better than the one on master. See the Traits and AlaScala branches for examples if you care.