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):
- 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.
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.
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...
-
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? -
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? -
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). -
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 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.
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?
Anthony
Surprised to say that I agree on just about everything you mentioned. I
would however love to see a useful autoloader included in core. I have
only one comment below.
- 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).
IMO, the interface should just define loadClass($class). It means that
spl_autoload_register has a fixed target for loading classes, and if we
want to support autoloading functions or whatever else later on, a new
interface can be added that would define the appropriate method eg.
loadFunction($function).
Cheers,
David
Well, the problem with adding methods later is that it will fatal any class
that implements the old one. A big no no.
You could get around that by doing something like traversable. Provide an
empty and non usable core SplAutoloader interface which is typehintable.
Then add a child SplClassAutoloader interface which defines loadClass.
That way, in the future a new child could be added SplFunctionAutoloader
which defines loadFunction.
By separating them, you maintain type hinting while enabling backwards
compatibility with existing code...
Just a thought.
Surprised to say that I agree on just about everything you mentioned. I
would however love to see a useful autoloader included in core. I have
only one comment below.
- 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).IMO, the interface should just define loadClass($class). It means that
spl_autoload_register has a fixed target for loading classes, and if we
want to support autoloading functions or whatever else later on, a new
interface can be added that would define the appropriate method eg.
loadFunction($function).Cheers,
David
David
Sorry, I just RE read your reply. that's basically what you said, so in
essence I agree...
Anthony
Well, the problem with adding methods later is that it will fatal any
class that implements the old one. A big no no.You could get around that by doing something like traversable. Provide an
empty and non usable core SplAutoloader interface which is typehintable.
Then add a child SplClassAutoloader interface which defines loadClass.
That way, in the future a new child could be added SplFunctionAutoloader
which defines loadFunction.By separating them, you maintain type hinting while enabling backwards
compatibility with existing code...Just a thought.
Surprised to say that I agree on just about everything you mentioned. I
would however love to see a useful autoloader included in core. I have
only one comment below.
- 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).IMO, the interface should just define loadClass($class). It means that
spl_autoload_register has a fixed target for loading classes, and if we
want to support autoloading functions or whatever else later on, a new
interface can be added that would define the appropriate method eg.
loadFunction($function).Cheers,
David
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.
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):
- 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?
- 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.
- 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.
- 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.
- 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. =)
- 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: guilhermeblanco@hotmail.com
São Paulo - SP/Brazil
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.
...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.phpYou 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.
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?
...snip...
- 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.
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?
Cheers,
David
Hi David,
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....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.phpYou 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.phpIf 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...
- 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
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_DEPRECATEDon 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: guilhermeblanco@hotmail.com
São Paulo - SP/Brazil
Hi
.. snip ..
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...
Would it not be possible that the autoloader mechanism catch all
exceptions made by any autoloader which fails to load a class. Only if
none of the register autoloaders can load the requested class the
autoloader mechanism throws an AutoloadException with all the catched
exceptions. This behaviour should be controllable by the users
environment. So if using an autoloader(e.g. from a library) which throws
an exception, but the user doesn't use exceptions in his environment, he
should be able to deactivate the exceptions thrown by the autoloader
mechanism. I think this behaviour has the greatest advantages for both
worlds. This one which needs exceptions to shut down the application in
an ordered way, and for these relying on PHPs way to handle the
autoloading error.
Cheers
Yes, but even in that case the autoloader would not be triggered to
load \Foo_Bar because if you already have used \Foo\Bar previously,
that would have loaded \Foo_Bar as well.
Well, my original point is that what if both are not defined in the
same file. It's not that far fetched of an idea. Let's say you're
using Zend Framework 2. So one part of your application uses the
\Zend\Registry class. Now, let's say you import a third party library
that was built with a dependency on the ZF1 \Zend_Registry class.
Without the require_once modification, it would fatal error since both
are not in the same file (even though a later autoloader knows how to
load Zend_Registry). Granted, it's a bit of a stretch, but it's not
out of the realm of possibility. And considering it's trivial to
avoid the error since all it takes is require_once, I think it's worth
fixing...
Would it not be possible that the autoloader mechanism catch all exceptions
made by any autoloader which fails to load a class. Only if none of the
register autoloaders can load the requested class the autoloader mechanism
throws an AutoloadException with all the catched exceptions. This behaviour
should be controllable by the users environment. So if using an
autoloader(e.g. from a library) which throws an exception, but the user
doesn't use exceptions in his environment, he should be able to deactivate
the exceptions thrown by the autoloader mechanism. I think this behaviour
has the greatest advantages for both worlds. This one which needs exceptions
to shut down the application in an ordered way, and for these relying on
PHPs way to handle the autoloading error.
Sure, it would be possible. But that would also be a BC break. It
may be worth it (not sure), but still. An interesting idea...
Not sure debug adds much here - if the class is not found, you'll be notified
by the engine anyway, so what's the added value?
I think what he was trying to do was provide an indicator of what the
mapped file that it tried to load was. So not throwing the exception
to show it failed, but throwing it to identify what the file path
should have been. Not arguing if it's a significant need, just what
was intended...
Am 11.11.2011 12:53, schrieb Anthony Ferrara:
Would it not be possible that the autoloader mechanism catch all
exceptions
made by any autoloader which fails to load a class. Only if none of
the
register autoloaders can load the requested class the autoloader
mechanism
throws an AutoloadException with all the catched exceptions. This
behaviour
should be controllable by the users environment. So if using an
autoloader(e.g. from a library) which throws an exception, but the
user
doesn't use exceptions in his environment, he should be able to
deactivate
the exceptions thrown by the autoloader mechanism. I think this
behaviour
has the greatest advantages for both worlds. This one which needs
exceptions
to shut down the application in an ordered way, and for these
relying on
PHPs way to handle the autoloading error.Sure, it would be possible. But that would also be a BC break. It
may be worth it (not sure), but still. An interesting idea...
If no one of the core devs has objections against it, I can write a RFC
for this feature!?
2011/11/11 Anthony Ferrara ircmaxell@gmail.com:
[snip]
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.phpWell, 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_BarThe point is that multiple classes can map to the same file.
Yes, but even in that case the autoloader would not be triggered to
load \Foo_Bar because if you already have used \Foo\Bar previously,
that would have loaded \Foo_Bar as well.
Hi!
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
Why this should even exist? It's never OK for generic autoloader to
produce any errors when it can't load the class, because it makes it
unusable with other loaders.
- 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.
Not sure debug adds much here - if the class is not found, you'll be
notified by the engine anyway, so what's the added value?
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
That's not what I see in the RFC:
\namespace\package\Class_Name
/path/to/project/lib/vendor/namespace/package/Class/Name.php
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.
The check is just a hash lookup, and you'll have to check for file
existence anyway...
Based on that, I'm pro to change the class to SplPsr0ClassLoader or
SplPsr0Loader.
This name looks way too L33t :)
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
- 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.Not sure debug adds much here - if the class is not found, you'll be
notified by the engine anyway, so what's the added value?
Right there will be fatal by engine anyway "Class does exist.."
Our approach with autoloading has been the following:
spl_autoload_register('MV_Autoload');
function MV_Autoload($className)
{
try {
MV_Loader::classImport($className);
} catch(MV_ClassNotFound_Exception $e) {}
}
It seems very wrong to me to generate a fatal error on class_exists() & al.
That's unless the spl_autoload_register() api changes and allows to deal with errors - require(), Exception, ...
Hi!
That's unless the
spl_autoload_register()api changes and allows to deal with errors - require(), Exception, ...
Autoloader shouldn't produce external errors - because if it's unable to
load class, others may be able. Also, it'd be quite hard to handle such
errors - you'd have to guard each operation that does anything with classes.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Anthony Ferrara wrote:
Since you asked me for feedback on how I would suggest improving the
RFC, so here it goes...
Silly question time ...
If I am reading all this correctly we are talking about how something is found
if I have not directly identified that I want to use it?
So if my base framework has already flagged which version of libraries I am
looking for I can continue to load them traditionally? Certainly identifying the
correct PEAR suit I want to work with has been important in the past as well as
the version of ADOdb and Smarty so identifying those on a target system that I
have less control over ... such as shared hosting! ... and supplying our own
bundle is only a necessary step if the required bundle is not available.
Although the safe way is to simply bundle everything anyway and not use
pre-loaded PEAR and the like.
Creating my own SPLClassLoader would be a way of implementing that if the
'default' one being offered is trying to load stuff that is not the correct
version? Or I just leave my own checks to find the correct versions depending on
which OS is running on the server?
require_once is almost essential in this case, and I was under the impression
that it was currently the economic way of checking that something was loaded
where the paths through libraries can get swapped? As usual there is much
pressure from a group of developers on why they have to have it, but no real
explanation on how it improves on the current methods? From my own view it is
accessing the correct PEAR classes which is the problem.
I have to admit that I simply 'switched off' since I could not see any point to
it and see it as just another esoteric noose rather than something I can see the
point of!
--
Lester Caine - G8HFL
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk//
Firebird - http://www.firebirdsql.org/index.php