Guilherme, Comments inline
> 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. Thanks. I didn't really intend for my other comments to get so... aggressive. I saw the proposal being (what looked to me) steamrolled through without addressing the issues that I saw with them. Rather than trying to focus on the technical and RFC issues, I chose to go another route. Which in retrospect was completely counter productive. I think my points still stand, but that's another topic for another day... > 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. I saw that. However that's kind of the point I'm making. The NORMAL mode is the default. And it's not obvious that it can cause the autoloader to not play gracefully with other code. So rather than reading the docs to realize what they want, most will use the default. Now, I completely understand the want to avoid the stat() call required by the silent mode. However, I would argue that other autoload strategies are more suited to those style roles than this would be (such as automap or PHPAB style autoloaders). So I'm not really sure it's in the scope of this implementation. But I would definitely suggest that if you still want NORMAL mode, that it should at least be non-default (SILENT should be the default, as it's the most flexible and considerate to other code). > 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 Well, as David pointed out, they do map to the same file. But even if they didn't, the following two classes would still map to the same file: \Foo\Bar \Foo_Bar The point is that multiple classes can map to the same file. > > 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. It may be slower (no more than a micro-optimization for most sites though). But it also plays nice. Perhaps a good compromise is to switch which you use based on the NORMAL vs SILENT modes. Where SILENT would use require_once and NORMAL would use require. Actually, at that point, it may be worth while renaming those two modes to COMPATIBILITY (for SILENT) and PERFORMANCE (for NORMAL), with the difference documented as such. The exact names are different, but that would at least better point out the distinction between them... > 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? I think that an exception to this request could be made for the DEBUG mode (as the way you're doing it is pretty much the only way you could identify that). Perhaps changing the exception to a warning or notice (as you wouldn't want that in prod), but I could live with the error in debug mode as it shouldn't be used outside of testing and dev work. I was more referring to errors/exceptions in normal usage... > 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. Agree with this point (and actually like it better). > 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. We could argue the merits of supporting a function autoloader in an OOP autoloader paradigm. But I don't think that's productive. I'm not really at heart disagreeing with what you're saying though. Perhaps it's worth a note in the RFC to this effect just so that there's a reference to point to later as to it was considered in the process... > 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. I'm just worried about what happens in PHP 6 or 7 or 8 when for some reason that we're not thinking about (or can't think about) it no longer becomes adequate. That's my concern. There are plenty of examples of features that were added because they seemed like a great idea at the time, but then time goes on and limitations and issues are found with it. I'm not saying it will definitely happen here, but the possibility is too great to completely ignore. I think it should at least be mentioned in the RFC. Even if it's just that the class name includes PSR0 to indicate that future implementations will be higher numbers to address possible changing requirements... > 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. It wasn't really clear when I read it (and still isn't really clear). This is a lesser issue, but just something that may be nice to have once it comes to documenting the class... > 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. Again, I'm in the middle on the function autloading being invalid here. I could see an argument made that it should be. Remember as I said before, you can implement OOP without objects. So just because you're using functions doesn't mean the code isn't OOP (and vise versa). But I wouldn't be completely upset to see it not included either as I don't wholely disagree with you. A simple system would be to add 2 interfaces, one for autoloaders in general, and another for class autoloaders that extend it (that way the door is open later)... But if not, I'm not really deadset on this. Just pointing an option... > 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. =) Perfect. I just wanted to see how it was inteded to be extended. Of course anyone who can read C would be able to tell that, but I'm a fan of explicitness over implication. So +1 on the docs effort. > 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. I wasn't really intending to point fingers or anything. Just trying to assert that we should be voting on an implementation and not a concept. I feel that the discussion and voting has been focused on the concept instead of the implementation (at least by many, not all), which is really one of the big issues that I have/had. So I think it's a good path... > 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. No argument at all. Just throwing it out there. > 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. Sounds good. I'm still against it in principle, but I'd much rather see a good implementation be put in then let a mediocre one get in because of political pressure (I understand the implementation/rfc wasn't complete when it came to a vote, just pointing out how/why I invested the time). Thanks for listening!!! Anthony > > 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
