-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Force loading of all aliases at startup #4
base: main
Are you sure you want to change the base?
Conversation
…using the AutoLoader to gracefully load classes on request.
Added unit tests for "autoload-mode" config variable. Changed how configurations are loaded/cast, to simplify adding additional configuration options.
I should point out I also modified how the Config class constants are defined, and how they get type-cast, because it annoyed me when adding new Config options. That's probably way out of scope and totally my personal preference. Let me know if that's an unacceptable change. |
First of all, let me thank you for trying out this package and creating such a great pull request. To the issue:Your observation is correct an expected, that this autoloader has to be the only loader for classes that have an alias defined. If this is not the case, then you will run into all sorts of issues. Let me explain the why in more detail by going to each case that can happen. 1. An alias class name is requested by the applicationGiven we have a class name If the application uses
That is the easiest and least problematic case. Everything is fine for this case. 2. A class name, which has an alias, is requested by the applicationGiven we have a class name If the application uses
These two steps need to be done be exactly one autoloader, otherwise you'll run into trouble. Here is why: Given we had a stack of two autoloaders
Everything is still working to some extend. If the application would now need However, consider code like this: interface A {
public function(NewClass $newClass);
}
class B implements A {
public function(LegacyClass $newClass) {}
} If OptionsThere are multiple options to fix this.
ConclusionThis alias loader package was designed to be able to deal with many classes and aliases. The option you introduced in the pull request, adds support for an edge case where only a few aliases are present. So it basically contradicts the initial intention of the package. This is the reason why I unfortunately cannot accept this PR as is. Nevertheless I'm here to support you. I planned to implement option 2 anyway, so that we benefit from this speed gain ourselves. I will start implementing this now. By doing so, I will introduce API, that you can also go for option 1, which isn't easily possible now because of limitations in the API. So I hope you won't choose option 3 and give this package another shot in the (near) future :) |
If you don't mind, if I will consider your changes to the |
@helhum thanks for coming back with the detailed explanation. The autoloader system isn't something I'm intimately familiar with. I fully agree with you about this being an edge-case for the ClassAliasLoader, and it definitely goes against the grain of the autoloader, but I don't think it's entirely out-of-bounds for this library as a feature.
I'm fully aware that our application code is still growing, so eventually it won't be practical to use this alias force-loading mechanism, and we will have to migrate our system to use the class-alias-loader's real autoloader. I mean, I could make them specify the aliases in an array format that's compatible with class-alias-loader, and then i could write something that loads those alias mappings from a list of files that i make people specify somewhere (composer.json perhaps?) ... but I'm sure you can see where I'm going with this :) This PR is option 3. It connects the dots between using I agree this feature is risky because of performance questions - you should absolutely know what you're doing before you opt into using the forced preloader. If you don't want to support that feature (and I can understand why) then I'm happy for you to discard this PR. Unfortunately for us, it's too dangerous to modify our autoloader(s) at this time, so we'll be opting to use this branch and (hopefully) maintain compatibility with class-alias-loader until the issues can be solved (which will either be when performance issues become noticeable, ApcClassLoader becomes fully redundant, or ClassAliasLoader gets an update that happens to make it compatible). Also, totally happy for you to cherry pick any of this PRs code, so no worries there. |
OK. As said, I'm happy to help here and make this package more useful for other cases. Do you have an opinion here? Besides that, I reconsider merging your feature to the current branch and do the refactoring/rewrite in another one. This way, you could rely on a released version of this package (let's say 1.1.0), while I'm developing 2.0, where I definitely drop all the case insensitive class name handling and raise the minimum PHP requirement. |
Give me some time however to get things straight here … |
@helhum No worries, thanks for considering this further. We're happy to use branched code, but it is even better when we can use proper releases ;) No rush. We're on PHP 5.6 at the moment, looking to upgrade to PHP7 in the next 6 months. |
92b5928
to
994ee9b
Compare
This PR adds the ability to configure Class Alias Loader so that all class_alias maps are loaded on boot, rather than as a pre-loader to the standard auto-loader.
This behaviour can be activated by specifying:
It merely cycles all the known class aliases (specified via Composer) and executes the class_alias call for each, providing a class name is not already loaded.
Risks
This autoloading method bypasses some of the safeties integrated in class-alias-loader:
But Why
We're using Symfony and want to relocate some classes from
A/B/C
toD/E/F
.This can be achieved simply enough using class_alias, but by using
class-alias-loader
this makes integration a hundred times cleaner, and in prototype and development this worked a charm.Unfortunately in production we also use
ApcClassLoader
, which wraps the Composer autoloader and caches the locations of Files which represent class names to be loaded.This significantly speeds up production class autoloading when your project is as enormo-huge as ours.
ApcClassLoader
needs to be at the top of the autoloading stack in order to intercept most (if not all) autoloader calls and load them from memory, rather than Composer.ClassAliasLoader
, which is loaded immediately after Composer's AutoLoader, also wants to be at the top of the stack.Once symfony has booted, we end up with a structure like this:
For whatever reason (I've not fully debugged it) the
ApcClassLoader
ended up obtaining a reference for theClassAliasName => RealClassFileName
, meaning that when it saw a reference to an Alias class name, it loaded the file for the REAL class name.This results in the real class file being loaded, but as
ClassAliasLoader
is never called the alias is never established.Other options
I tried putting the
ClassAliasLoader
in front of theApcClassLoader
, but the issue still occurred. It was also a performance problem to place that autoloader first when theApcClassLoader
should be prioritised in 99.999% of class loading cases.I also tried letting the
ApcClassLoader
fail over to theClassAliasLoader
(as its next in the autoloader stack) but the ApcClassLoader was still ending up with the rogue class reference.Besides this, we have some super bizzarro class referencing code that makes me wonder if it's even safe to lazy-load the class aliases on auto-load request.
Rather than poke these bears, I've chosen to just make
class-alias-loader
force-load all class_alias mappings at boot, and avoid heavy modifications to our complex applications class autoloader.