Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:56267
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.42 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAAyV7nEi+1XzYryoE9tg48jZMMPe1psz=VWDkGSzEgAwBKs9pw@mail.gmail.com>
References: <CAAyV7nEi+1XzYryoE9tg48jZMMPe1psz=VWDkGSzEgAwBKs9pw@mail.gmail.com>
Date: Fri, 11 Nov 2011 00:31:35 -0200
Message-ID: <CAFp73XuQGG9mEZUZ2OK1yLkBiFDcPwOxytQ80D=xZUafEv20tw@mail.gmail.com>
To: Anthony Ferrara <ircmaxell@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: SPLClassLoader RFC Thoughts And Improvements
From: guilhermeblanco@gmail.com ("guilhermeblanco@gmail.com")

Hi Anthony,

Thanks immensely for your input.
Without such action, it's extremely hard to improve the RFC. =3D)
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> wrot=
e:
> 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. =C2=A0I don't think that I
> can vote yes for this RFC in principle (I just don't agree this
> belongs in core). =C2=A0However, 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. =C2=A0I s=
ay
> 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. =C2=A0This means that it should **never*=
*
> throw an error under any circumstances. =C2=A0In order to achieve that,
> three things would need to happen:
>
> =C2=A0a) The code that includes the file would need to be wrapped in a
> file_exists call to ensure that the include would actually work. =C2=A0Th=
is
> 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 va=
lues:
- 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 =3D new \SplClassLoader(\SplClassLoader::MODE_SILENT |
\SplClassLoader::MODE_DEBUG);
// ...

>
> =C2=A0b) The code that includes the file should be changed to require_onc=
e
> to eliminate including the same file twice when different classes map
> to the same file. =C2=A0For example, this should not fail (assuming both
> classes are not in the same file):
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0new \Foo\Bar\Baz;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0new \Foo\Bar_Baz;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0Both map to the same file, but are distinct cl=
asses 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; =3D> [path]/Foo/Bar/Baz.php
new \Foo\Bar_Baz; =3D> [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. =3D)

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.

> =C2=A0c) =C2=A0No exceptions, warnings, notices, etc should be thrown und=
er any
> circumstances *when loading a file*. =C2=A0You 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). =C2=A0And considering that we can use ErrorExceptions, raising
> PHP Errors would cause the same problem. =C2=A0Note that this does not
> affect the included file as that can do any error. =C2=A0Just the
> autoloader itself shouldn't. =C2=A0This means that setMode should be
> removed since it would cause the autoloader to stop from "playing
> nice". =C2=A0This seems like user choice, but what if a library ships in
> normal mode (which is default now) and these issues occur? =C2=A0My view =
is
> that it should *always* be silent. =C2=A0PHP has mechanisms for handling
> not-found classes. =C2=A0Autoloading 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. =C2=A0I know it appears that PSR-0 is 100% sufficient now, but
> there will come a time when it is no longer so. =C2=A0A perfect example o=
f
> that is the discussion that was on the list in the past few
> weeks/months about adding function autoloading. =C2=A0What happens at tha=
t
> point, how could/should this implementation adapt and evolve? =C2=A0I wou=
ld
> suggest not tackling specific possible changes, but more on how
> changes would be handled. =C2=A0Would you add new methods? =C2=A0New clas=
ses?
> Change APIs? =C2=A0How would you maintain backwards compatibility? =C2=A0=
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. =C2=A0If we add this PSR-0 style autoloader, can
> other style autoloaders be included? =C2=A0I know some will say "do any
> others need to be included". =C2=A0But what about things like the automap
> extension (which maps classes to files via an array list)? =C2=A0How shou=
ld
> 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. =C2=A0=
An
> example would be implementing an interface for the autoloader which
> defines something like load($class). =C2=A0The problem there is that if
> function autoloading is added, the interface won't be able to support
> it. =C2=A0So 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. =C2=A0For example, if users wanted to
> change the behavior, can they just subclass and override certain
> methods? =C2=A0Are there any "dangerous" methods that shouldn't be
> overriden? =C2=A0Should 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? =C2=A0The 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. =3D)

>
> 6. The RFC should contain a proposed patch (or at least reference
> implementation) prior to being voted upon. =C2=A0That way everybody knows
> what's being voted on, rather than a wandering text. =C2=A0Because
> 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). =C2=A0That patch should not change fundamentally (aside fro=
m
> tweaks or bug fixes) after the vote starts. =C2=A0If 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. =C2=A0You might want to consider adding __invoke()
> support so that you don't need to change spl_autoload_register at all.
> =C2=A0That 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,

--=20
Guilherme Blanco
Mobile: +55 (11) 8118-4422
MSN: guilhermeblanco@hotmail.com
S=C3=A3o Paulo - SP/Brazil