[WIP] Disallow non-static calls to the constructor

[pmmaga]

This is an initial implementation of a future RFC to disallow non-static calls to constructors. With this PR I hope to get some initial feedback on the feasibility of the approach on a technical level.

[nikic]

Let's ask some of the usual suspects (@Ocramius @nicolas-grekas) whether this is going to Break All The Things.

[nikic]

This should be part of the else above, to reduce cost.

It might make sense to actually move this into get_method, which will prevent acquiring __construct as an ordinary method completely. But maybe that will interfere with parent::__construct?

Right now this is probably missing constructor calls via call_user_func.

[pmmaga]

The reason why I left it outside of the else was that even though the CONST case should be caught by the compile time check, I wasn't completely sure if it would cover all the cases. I'll try to check those other cases/options.

[nicolas-grekas]

Yes, this is going to break code out there for sure. Calling the constructor as a method e.g. in a __clone() method is OK. Calling it also internally to reinit an object also is, more generally. Yes, you can decide it's not fine, but this would be forcing calisthenics for the sake of them, IMHO. This won't solve real life issues.

[nikic]

For context, this recently came up on Reddit with regard to property immutability. Calling __construct() manually gives a trivial way to bypass any immutability guarantees. The readonly RFC would fix this, as it is not visibility based.

Not sure if there's really a problem to solve here though, as manually calling __construct() is on the same level of shenanigans as using reflection or closure rebinding, both of which will already allow you to modify arbitrary interior state.

It's an oddity for sure, but not exactly something you can do accidentally and introduce a bug.

[nicolas-grekas]

Not sure if there's really a problem to solve here though, as manually calling __construct() is on the same level of shenanigans as using reflection or closure rebinding, both of which will already allow you to modify arbitrary interior state.

Totally this.
What matters is that there are no contracts to mutate a properly designed public API. Then, if ppl trick the engine to still mutate such objects, that's at their own risks, and that's totally fine once this is said.

I actually much prefer a hackable engine where the consumer has most of the power rather than a restrictive engine that allows authors to takes power from users. Only the final consumer knows what it wants to do. Let's them take their risk. Of course, they should not blame the authors if they do so. I think the situation is clear enough for both sides with the current state of things.

[Ocramius]

I do call __construct again (after instantiation), but on instance scope, in scenarios similar to unserialize(), clone and ReflectionClass#newInstanceWithoutConstructor().

Calling self::__construct() is not something I saw in the wild thus far...

[pmmaga]

Although it would still be possible to modify the state with other tools, this would forbid the most lazy one, and the one that can easily be done by someone new to the language by mistake.

I actually thought this up before the reddit post when I was looking at https://wiki.php.net/rfc/disallow-multiple-constructor-calls which has a similar purpose but with a different approach. I think that enforcing the static call (self::__construct()) in the legitimate cases would be the way to go. And it makes more sense, you're modifying the current instance, and the static call way makes it clear.

I'm still unsure if I will actually pursue this RFC, but thanks for the initial feedback.

[nicolas-grekas]

@pmmaga you build on self::__construct() being written using static syntax when in fact $this is forwarded, isn't it? This wouldn't forbid anything from the inside of the class, but would from the outside. Got it.

This would require ppl that do $this->__construct() or [$this, '__construct'] internally to rewrite these as self::__construct(), but this would be totally seamless, so no big deal?

I think this clears my initial no-go reaction, yet I'm still not comfortable with the base reasoning: enforcing calisthenics won't help solve real-world problems.

this would forbid the most lazy one, and the one that can easily be done by someone new to the language by mistake.

I'm not sure anyone new to the language ever thought this would be a possibility. At least I never encountered anyone that did this by mistake. That's more something that you learn in "advanced tricks - don't do at home" articles to me. Did you?

[pmmaga]

I've decided to not pursue this change anymore. Although I think it would force some "correctness", the fact that other methods still allow unwanted mutations would only bring "harder-to-mutate" objects which no one is really asking for. :)