Hi Anthony, Thanks immensely for your input. Without such action, it's extremely hard to improve the RFC. =) Awesome action from your side. I'll place my comments inline.
On Thu, Nov 10, 2011 at 1:26 PM, Anthony Ferrara <ircmax...@gmail.com> wrote: > Guilherme et al, > > Since you asked me for feedback on how I would suggest improving the > RFC, so here it goes... > > I think I need to make one thing clear first. I don't think that I > can vote yes for this RFC in principle (I just don't agree this > belongs in core). However, with that said if the RFC was clarified > and takes care of the issues that are listed here, I will stop > actively opposing it and may even consider removing my no vote. I say > this not because I want to emphasize anything, but because I want to > make sure that expectations are managed and that nobody is going to > get mad at me for not changing my vote if everything here is > completed. > > Now, with that disclaimer behind us, let's move on to how I would like > the RFC changed (In no particular order): > > 1. The implemented solution **must** under all circumstances play nice > with other autoloaders and code. This means that it should **never** > throw an error under any circumstances. In order to achieve that, > three things would need to happen: > > a) The code that includes the file would need to be wrapped in a > file_exists call to ensure that the include would actually work. This > will prevent issues with different tree structures causing errors to > be thrown. This is actually already supported. The SplClassLoader$mode does exactly that. The $mode can be one of these values: - MODE_NORMAL: Throws error while attempting to include an unexistent file - MODE_SILENT: Does the is_file() check before attempting to require. This helps the case where you may have multiple SplAutoloader instances. - MODE_DEBUG: This one can work together with the other two, allowing a validation of class/interface/trait presence in the file. Basically, it requires the file and then checks if the item exists in scope. This is why you could implement the SplClassLoader like this: $loader = new \SplClassLoader(\SplClassLoader::MODE_SILENT | \SplClassLoader::MODE_DEBUG); // ... > > b) The code that includes the file should be changed to require_once > to eliminate including the same file twice when different classes map > to the same file. For example, this should not fail (assuming both > classes are not in the same file): > > new \Foo\Bar\Baz; > new \Foo\Bar_Baz; > > Both map to the same file, but are distinct classes in PHP. > By changing it to require_once, it'll realize it already included the > file and then skip over that and let another class have its shot. > Actually they do not map the same file. Here is the path of each one: new \Foo\Bar\Baz; => [path]/Foo/Bar/Baz.php new \Foo\Bar_Baz; => [path]/Foo/Bar_Baz.php You may now be in doubt if PEAR1 style is supported. Yes, it is. The point is that code detects for the presence of namespace separator and if it is found, it considers the namespace separator as directory separator; if not, it considers the "_" char as directory separator. We decided that because PEAR1/Zend1/Doctrine1/Symfony1 (and many others) would not be supported if we didn't do that way. It's the solution for BC. Since most of the mentioned ones are already in another major version, consuming PHP 5.3 namespaces, then the default behavior would work nifty. =) Regarding the require_once change, it's not really needed. The reason is because class loading would never be triggered again if the class/trait/interface is already loaded. The require_once is extremely slow than require due to this check. But I really don't have strong feelings to change that is many people find it necessary. > c) No exceptions, warnings, notices, etc should be thrown under any > circumstances *when loading a file*. You could have it throw an > exception (or warning, whatever) if the path passed to the constructor > is invalid, but when loading a class it should never throw one (which > would cause issues or prevent other autoloaders from functioning > properly). And considering that we can use ErrorExceptions, raising > PHP Errors would cause the same problem. Note that this does not > affect the included file as that can do any error. Just the > autoloader itself shouldn't. This means that setMode should be > removed since it would cause the autoloader to stop from "playing > nice". This seems like user choice, but what if a library ships in > normal mode (which is default now) and these issues occur? My view is > that it should *always* be silent. PHP has mechanisms for handling > not-found classes. Autoloading is one, and fatal errors is another. > It's not the job of the autoloader to throw errors... Hm... so there should never have the normal available? I need to think over this again then. While I tend to agree with autoloader never triggering errors or exceptions, the debug mode is the unique way to notice if user during developer haven't done any mistake. Maybe we can only keep the RuntimeException and debug mode possibility, remove the normal and keep it always silent. What do you think? > > 2. The RFC *needs* to address how the implementation may/should evolve > over time. I know it appears that PSR-0 is 100% sufficient now, but > there will come a time when it is no longer so. A perfect example of > that is the discussion that was on the list in the past few > weeks/months about adding function autoloading. What happens at that > point, how could/should this implementation adapt and evolve? I would > suggest not tackling specific possible changes, but more on how > changes would be handled. Would you add new methods? New classes? > Change APIs? How would you maintain backwards compatibility? Can you > maintain BC? First I'll expose one of the things I've been thinking, then I'll discuss the other points. I *do* agree the SplClassLoader should be renamed to something that reminds the PSR-0. Not because of ego, but others highlighted that another PSR in the future that change the behavior of class loader (ie, removing the PEAR1 style support). This would require a change in SplClassLoader which might break existent applications. It's not good. Just like a normal community process, newer recommendations means that they deprecate the previous ones. That way code that still follows the old approach should still be supported, but throwing E_DEPRECATED. Based on that, I'm pro to change the class to SplPsr0ClassLoader or SplPsr0Loader. Now getting back to your point, the PSR-0 and this RFC refers exclusively to OO paradigm. The idea would cover classes, interfaces and traits only. For function autoloading, it's part of another paradigm, so you might be in a procedural paradigm in your code, so the usage of an OO definition is a bit weird to me. Mixing paradigms is not healthy, it increases the complexity of the code and also reduces the readability. Regarding possible new PSRs, the main focus of the group is about good practices and standardization of PHP code. We have an extremely few suggestions that could be portable to C code. All of them (except PSR-0) were not even discussed, but I can give you some idea of possible areas: Cache layer, enhancements to PDO and new data structures. I can't see by now other areas, but they may exist. Anyway, if we get back to the autoloading discussion maybe in 5 years, then what people have pointed our and I previously referred is valid. The class name would have to change already in this RFC, and the behavior would act like I mentioned too. E_DEPRECATED on previous loader and implement the new one. > > 3. The RFC should at least address how other autoloading schemes would > fit into the picture. If we add this PSR-0 style autoloader, can > other style autoloaders be included? I know some will say "do any > others need to be included". But what about things like the automap > extension (which maps classes to files via an array list)? How should > that fit into the picture? I already mentioned that in RFC. You're able to instantiate and register as many autoloaders as you want. Also, you can have your own implementation by implementing the contact (SplAutoload) or extending the default behavior (SplClassLoader, SplPsr0ClassLoader, SplPsr0Loader or whatever name it becomes). The class map would work perfectly, and the only necessary action would be to call the ->add() method. > > 4. The RFC should avoid implementing any pattern or style that may > make future feature addition difficult or pose risks towards such. An > example would be implementing an interface for the autoloader which > defines something like load($class). The problem there is that if > function autoloading is added, the interface won't be able to support > it. So it's stuck in a hard place between changing an implemented > interface (which will bork code significantly on update) and adding a > new interface (which would be the lesser of evils, but would just add > to the deprecated cruft). OO theory defines that whenever you want to have a contract to be followed by any implementation, an interface is required. The contract is something useful that enforces that way you receive would have at least what it was expected/used internally. So, if we agree in the future that the chosen implementation would not contain the methods ->register/->unregister, and spl_autoload_register would accept SplAutoload instances, then the contract is more than required. Regarding the function autoloading, as I discussed previously, it is invalid in this paradigm. I don't see how the contract would change in the future. > > 5. The RFC *must* talk about how user-land classes can and should > extend this core implementation. For example, if users wanted to > change the behavior, can they just subclass and override certain > methods? Are there any "dangerous" methods that shouldn't be > overriden? Should any of the C functions (such as get_filename) be > implemented as a method (possibly protected) to allow for extending > classes to change that behavior? The important thing is that this > will need to be documented prior to release, and it will make our > lives easier documenting recommended best practices if it's already at > least discussed in the RFC. The RFC already defined the visibility of its members, so the methods marked as public/protected are the ones extendable. The ones that can't be extended are marked as private, since its overall behavior cannot be changed. Of course I can expand the RFC by adding a section describing how users can implement their own SplAutoload or even extend the SplClassLoader (ok... type the final name here). Thanks for the tip. I added to my TODO. Should be included until monday. =) > > 6. The RFC should contain a proposed patch (or at least reference > implementation) prior to being voted upon. That way everybody knows > what's being voted on, rather than a wandering text. Because > ultimately it doesn't matter what's written in the RFC as the code > will always win (code wins over docs under all circumstances, as code > doesn't lie). That patch should not change fundamentally (aside from > tweaks or bug fixes) after the vote starts. If it does change, the > vote should be restarted (or at least extended with a note made about > it) as it's not really fair to ask people to vote on a target, and > then move that target. The RFC was not ready when it was opened for voting. I didn't open it. I tried my best to have everything wrapped before the voting starts, but I couldn't. Now we can't get back, but we can for sure reset and vote on the final version of proposal. I'm just taking this time to discuss with interested people how can we make this stable to then propose a new voting round. > > > > Now, one other point. You might want to consider adding __invoke() > support so that you don't need to change spl_autoload_register at all. > That way, you could support spl_autoload_register(new > SplClassLoader()); without needing to touch existing functionality... > > > Thoughts? I tend to reject enforcing the implementation of magic methods. It would be necessary to be part of SplAutoload contract, something that seems weird IMHO. I prefer to stick to the original proposal on this one, but I'm -0 on this one. > > Anthony > PS: Thanks again for investing your time reviewing the RFC and providing your expectations. Without a community review, it's hard to make any RFC stable. I think more people can/should comment on this, and if we find a reasonable consensus, update the RFC with the changes and reopen the poll as soon as possible. I'll be waiting for your feedback. Cheers, -- Guilherme Blanco Mobile: +55 (11) 8118-4422 MSN: guilhermebla...@hotmail.com São Paulo - SP/Brazil -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
