Re: Re: SPLClassLoader RFC Thoughts And Improvements

Hi David, On Fri, Nov 11, 2011 at 1:31 AM, David Muir <davidkmuir@gmail.com> wrote: > On 11/11/2011 01:31 PM, guilhermeblanco@gmail.com wrote: >> 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 <ircmaxell@gmail.com> wrote: >> > > ...snip... > >>>  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. =) > > This is news to me, and even the RFC states: > Each “_” character in the CLASS NAME is converted to a > DIRECTORY_SEPARATOR. The “_” character has no special meaning in the > namespace. > > I believe you're confusing it with the namespace portion: > new \Foo_Bar\Baz; => [path] /Foo_Bar/Baz.php > in which case the _ is retained. > Otherwise: > new \Foo\Bar_Baz; => [path] /Foo/Bar/Baz.php > new \Foo_Bar\Baz_Waz => [path] /Foo_Bar/Baz/Waz.php > > If what you are saying is true, then the RFC is incorrect, and should be > updated to included the correct rules. You're completely right. Sorry for my mistake. I overlooked it tonight... it might be my lack of some sleep these days. =\ > >> >> 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. > > That was true 6 years ago. Benchmarks from 4 years ago, it was the other > way around. Has it flipped back again? > I remember that 2 years ago we did benchmarks around require/require_once and the latter was slower by a factor of 30%. > ...snip... > >>> 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. > > But the only contract that is needed by spl_autoload_register is the > load method. Agree. > > The register and unregister methods are not needed by any other entity > that I am aware of, and are only there as convenience methods for the > user, and therefore don't belong in the interface for a generic > autoloader. A second interface could be added for self-registering > autoloaders, but that begs the question, why should an autoloader know > how to register itself with a particular loading stack (sure the spl > autoloading stack is the only one we have, but still...)? There's no > discussion in the RFC showing why those methods should be part of an > autoloader interface. > The same applies to the setMode() and add() methods. What contract are > they fulfilling? The add was a useful inclusion to keep consistency of that every SplAutoload should be able to deal with > 1 namespace. It's not mandatory and if everyone agrees that it should be taken out, I'll remove. The setMode method becomes invalid if we drop the MODE_NORMAL support. Of course there will have the MODE_DEBUG yet to address, but it can be removed from SplAutoload contract. > > Cheers, > David > -- Guilherme Blanco Mobile: +55 (11) 8118-4422 MSN: guilhermeblanco@hotmail.com São Paulo - SP/Brazil