Re: [midgard-dev] [midgard-commits] r22673 - in trunk/external-tools/mvc_installer: bin lib lib/exporter lib/installer lib/resolver
-
Alexey Zakhlestine
Re: [midgard-dev] [midgard-commits] r22673 - in trunk/external-tools/mvc_installer: bin lib lib/exporter lib/installer lib/resolver
Wed June 24 2009 07:45:07 UTCHi. I read code a bit and… well, here are some thoughts/questions about it:
1) wouldn't it be better to use autoload? it would allow to safely use
"require" instead of "require_once" which gives noticeable
performance-boost
2) midgard2_installer_installer::get() is declared to return
reference. it is not needed, as php5 always returns objects as
references (explicit reference is a bit less efficient even)
3) midgard2_installer_exception sounds like too-general solution. I
think it would make sense to use midgard2_installer_logic_exception
and midgard2_installer_runtime_exception, at least (extending SPL's
LogicExtension and RuntimeExtension)
4) exception_error_handler throws ErrorException which is not defined.
this is probably just a typo
5) sometimes you declare methods as "function methodname()" and
sometimes as "public function methodname()". probably everything
should use explicit variant
6) I am not sure, that "date_default_timezone workaround" is a good
idea. system administrator should set timezone in php.ini and we
should not silence appropriate warning
p.s. I'd be glad to help with this installer :)
On Wed, Jun 24, 2009 at 12:31 AM,
rambo<midgard-commits@lists.midgard-project.org> wrote:
> Author: rambo
> Date: Tue Jun 23 22:31:06 2009
> New Revision: 22673
> URL: http://trac.midgard-project.org/changeset/22673
>
> Log:
> infrastructure code for the midgard2-installer, refs #1196
>
--
Alexey Zakhlestin
http://www.milkfarmsoft.com/
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev -
-
Re: [midgard-dev] [midgard-commits] r22673 - in trunk/external-tools/mvc_installer: bin lib lib/exporter lib/installer lib/resolver
Wed June 24 2009 10:20:06 UTC-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160
Alexey Zakhlestin wrote:
> Hi. I read code a bit and… well, here are some thoughts/questions about it:
>
> 1) wouldn't it be better to use autoload? it would allow to safely use
> "require" instead of "require_once" which gives noticeable
> performance-boost
>
I do not want to mess with any other autoloader that might be active
(for example a web frontend), actually the require_once:s can be
replaced with require anyway since I only call them when I know I need
to load the file.
Shouldn't the explicit paths eliminate the performance problem of
checking all include paths whether file has been included or not ?
> 2) midgard2_installer_installer::get() is declared to return
> reference. it is not needed, as php5 always returns objects as
> references (explicit reference is a bit less efficient even)
>
Old habits... Also used as flag for anyone reading the code that this is
indeed supposed to be a reference to singleton stored elsewhere.
Besides isn't PHPs automagic reference COW ? In this case we definitely
do not want that since it's supposed to be a singleton.
The whole automagic references business is a bit confusing (It doesn't
seem very consistent to me), I don't think I have ever seen a truth
table anywhere of how it's supposed to work (let alone how it actually
works)
> 3) midgard2_installer_exception sounds like too-general solution. I
> think it would make sense to use midgard2_installer_logic_exception
> and midgard2_installer_runtime_exception, at least (extending SPL's
> LogicExtension and RuntimeExtension)
>
Point, OTOH the codes are the meat of the matter since they're used as
shell exit codes to tell non-php frontend talking with the CLI what's
going on.
> 4) exception_error_handler throws ErrorException which is not defined.
> this is probably just a typo
>
Nope:
http://fi.php.net/manual/en/class.errorexception.php
>
> 5) sometimes you declare methods as "function methodname()" and
> sometimes as "public function methodname()". probably everything
> should use explicit variant
>
Point.
>
> 6) I am not sure, that "date_default_timezone workaround" is a good
> idea. system administrator should set timezone in php.ini and we
> should not silence appropriate warning
>
The admin has plenty of chances to fix this later and we don't really
care about the timezones correctness but it's major plus if it's the
same as the system timezone (which is why it's not set to UTC
explicitly), reason for system TZ is that some temp and backup files are
crated with human readable timestamps.
And due to the ErrorException approach the warning will throw an
exception that is not trapped since IMO it's stupid to trap all
ErrorExceptions there and then check it's only this warning, if it's not
re-throw, otherwise output and continue.
>
> p.s. I'd be glad to help with this installer :)
>
Great ! dependencies resolving is going to be "interesting"
I'm looking at this in detail again next week ("real work" calls for
now) but we can chat about the ideas/design in jabber or irc before that.
@bergie also has notes of our planning session from little over week ago
but he forgot to update the mRFC/#1196 with the full notes.
/Rambo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFKQfm5k2FlZlXdE74RA8I1AJ4gw3/jE9ntP/q0O2Q+J69V2QU1WgCfZbde
yozXWYBGGYxt971tJXc4nwQ=
=59cX
-----END PGP SIGNATURE-----
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev -
-
Re: [midgard-dev] [midgard-commits] r22673 - in trunk/external-tools/mvc_installer: bin lib lib/exporter lib/installer lib/resolver
Wed June 24 2009 11:30:06 UTCOn Wed, Jun 24, 2009 at 2:02 PM, Eero af
Heurlin<eero.afheurlin@nemein.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: RIPEMD160
>
> Alexey Zakhlestin wrote:
>> Hi. I read code a bit and… well, here are some thoughts/questions about it:
>>
>> 1) wouldn't it be better to use autoload? it would allow to safely use
>> "require" instead of "require_once" which gives noticeable
>> performance-boost
>>
>
> I do not want to mess with any other autoloader that might be active
> (for example a web frontend), actually the require_once:s can be
> replaced with require anyway since I only call them when I know I need
> to load the file.
that won't be a problem, as autoloaders can (ans SHOULD) be chained
using http://docs.php.net/spl_autoload_register
> Shouldn't the explicit paths eliminate the performance problem of
> checking all include paths whether file has been included or not ?
require + absolute path = maximum performance
require_once (in 5.2.x and earlier) still gives performance penalty
anyway, I think autoload will result in cleaner code
>> 2) midgard2_installer_installer::get() is declared to return
>> reference. it is not needed, as php5 always returns objects as
>> references (explicit reference is a bit less efficient even)
>>
>
> Old habits... Also used as flag for anyone reading the code that this is
> indeed supposed to be a reference to singleton stored elsewhere.
>
> Besides isn't PHPs automagic reference COW ? In this case we definitely
> do not want that since it's supposed to be a singleton.
>
> The whole automagic references business is a bit confusing (It doesn't
> seem very consistent to me), I don't think I have ever seen a truth
> table anywhere of how it's supposed to work (let alone how it actually
> works)
copy-on-write in applied to pass-by-value, not to pass-by-reference
objects+references question is documented in the manual pretty good,
actually: http://docs.php.net/spl_autoload_register
basic idea is, that in PHP5 variable is a little-proxy which forwards
all calls to actual object. when you pass object-var to function, the
function will have a new "proxy var" but it will still point to the
same object. This proxy-var, by the way, will use copy-on-write
semantics, which means that it will be extremely efficient (no
preliminary memory allocation).
there was some good presentation about this, but I can't find it at the moment.
probably I can draw something to describe this in details
>> 3) midgard2_installer_exception sounds like too-general solution. I
>> think it would make sense to use midgard2_installer_logic_exception
>> and midgard2_installer_runtime_exception, at least (extending SPL's
>> LogicExtension and RuntimeExtension)
>>
>
> Point, OTOH the codes are the meat of the matter since they're used as
> shell exit codes to tell non-php frontend talking with the CLI what's
> going on.
My thought was, that there might be cases when exceptions would,
actually, need to be caught.
In case of runtime-exceptions there are a lot of approaches of
sane-handling (sometimes it makes sense to retry, sometimes it makes
sense to try another option).
And in case of logic-exceptions the best approach is to log error and
gracefully shutdown
>> 4) exception_error_handler throws ErrorException which is not defined.
>> this is probably just a typo
>>
>
> Nope:
> http://fi.php.net/manual/en/class.errorexception.php
interesting… I wonder why I never saw it.
>> 6) I am not sure, that "date_default_timezone workaround" is a good
>> idea. system administrator should set timezone in php.ini and we
>> should not silence appropriate warning
>>
>
> The admin has plenty of chances to fix this later and we don't really
> care about the timezones correctness but it's major plus if it's the
> same as the system timezone (which is why it's not set to UTC
> explicitly), reason for system TZ is that some temp and backup files are
> crated with human readable timestamps.
>
> And due to the ErrorException approach the warning will throw an
> exception that is not trapped since IMO it's stupid to trap all
> ErrorExceptions there and then check it's only this warning, if it's not
> re-throw, otherwise output and continue.
>
>>
>> p.s. I'd be glad to help with this installer :)
>>
>
> Great ! dependencies resolving is going to be "interesting"
>
> I'm looking at this in detail again next week ("real work" calls for
> now) but we can chat about the ideas/design in jabber or irc before that.
>
> @bergie also has notes of our planning session from little over week ago
> but he forgot to update the mRFC/#1196 with the full notes.
>
>
> /Rambo
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.7 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFKQfm5k2FlZlXdE74RA8I1AJ4gw3/jE9ntP/q0O2Q+J69V2QU1WgCfZbde
> yozXWYBGGYxt971tJXc4nwQ=
> =59cX
> -----END PGP SIGNATURE-----
> _______________________________________________
> dev mailing list
> dev@lists.midgard-project.org
> http://lists.midgard-project.org/mailman/listinfo/dev
>
--
Alexey Zakhlestin
http://www.milkfarmsoft.com/
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev -
