On Sat, Jul 26, 2014 at 12:29 AM, Stas Malyshev <smalys...@sugarcrm.com>
wrote:
> Hi!
>
> > client) I see no reason to keep the limitation in
> > the ReflectionClass::newInstanceWithoutConstructor() and allowing the
> > instantiation of internal classes will provide a clean upgrade path to
> > doctrine and co.
>
> I think we should ensure the objects will be in safe (i.e. no-crashing)
> state following create_object even if ctor was not called. The question
> is if we can ensure that and what would be the effort.
>
we already discussed this via moving the init logic to the create/clone
handlers from __construct for such classes.
>
> As for upgrade path for doctrine, etc. - if we're talking about
> something like test mocking, wouldn't something like:
>
> class Mock_Stub_FooClass extends FooClass {
> function __construct() {}
> }
> $foo = new Mock_Stub_FooClass();
>
yeah, that would work ofc, but as these libs seems to have instanitate
arbitrary classes, that would require either generating files on the fly
and including them or simply evaling them, but of those are a bit dirtier
than using Reflection for the same job.
>
> solve the problem? I understand it's not argument against
> newInstanceWithoutConstructor as essentially it does the same, just
> saying newInstanceWithoutConstructor strictly speaking would not be
> necessary?
>
true, but it can also be used to argue for loosening the restriction, why
restrict something from Reflection, which is already possible from simple
class extension.
>
> I think an approach for this would be to take pull 733 and make a script
> (or a test) that takes all the classes we define internally (may be a
> bit hard since some extensions need external libs, but we can check
> those manually) and instantiate them using this (and all other
> non-standard) method and try to call random methods and see if we get
> any crashes.
Julien already went over the potential classes and did the same thing
(calling methods over them), the list of affected classes is just the
previous one in this thread.
> But we should be reasonably sure at least most common ones
> would not crash if we enable this.
>
this change wouldn't introduce any crash, which isn't already possible
through a subclass not calling parent::__construct, ofc. if we can fix the
crashes before the final release that's nice, but I wouldn't say it is
mandatory, how it was hidden for years(and even so that the remote trigger
from unserialize is fixed).
>
> Another question would be if objects like COM would react to this
> properly. But if not it would be possible to make them final too.
>
yeah, but I would rather fix them than move to final, as I mentioned
before, it seems that some/most of the internal classes with final
constructor are final so that this problem can't occur, but this doesn't a
nice thing from OOP POV and also will cause problems if/when we introduce a
reflection method removing final from classes/methods (this was already
proposed not that long ago with a working patch but was turned down because
other reasons).
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
