On Thu, 18 Jun 2020 at 18:33, Benas IML <benas.molis....@gmail.com> wrote:
> Hey Andreas, > > That is incorrect. Adding an explicit `: void` does change behavior since > only then the check (whether something is being returned) is enforced. This > allows PHP 8 projects to take advantage of this enforcement while being > respective to older codebases. > > Ok. I read more carefully now. But this distinction would only apply in PHP 8. And the only difference here is whether returning a value is just deprecated or fully illegal. In PHP 9, constructors with ": void" would be the same as without ": void". So long term it will become a "meaningless choice". Or what am I missing? > And well, the explicit `: void` declaration also helps your code to be > more consistent with other methods ;) > Except consistency with existing constructors in other packages which choose to not add ": void" to constructors. > > Without an explicit `: void` return type declaration, `void` rules are not > enforced on constructors/destructors and will not be until PHP 9.0 (which > will probably release in 5 years). > > Moreover, saying "it forces organisations, frameworks or the php-fig group > to introduce yet another coding convention" is a complete exaggeration. In > fact, even now there are no PSR conventions that specify how and when to > write parameter/return/property type hints. > Either it is enforced in a "code convention", or it is up to every single developer and team, and we get silly arguments between developers in code review whether or not this should be added. Or we get git noise because one developer adds those declarations, and another removes them. > > Also, it's important to understand that PHP libraries are really really > slow at starting to **depend** on new PHP versions. It will probably take a > few good years (if not more) until first few libraries start requiring PHP > 8.0. As a matter of fact, even Symfony framework is still requiring PHP > 7.2.5 and cannot take advantage of its newer features (e. g. typed > properties). > So if you want to support PHP 7, you cannot add ": void". If you only care about PHP 9, you don't need to add ": void" because it is pointless. Any convention would probably discourage it to be more universally usable. > > Last but not least, just to reiterate, the `: void` return type is > optional and you don't need to specify it. > Exactly my point. "Optional" means people will make arbitrary choices and argue about it, or look for a convention. Greetings Andreas > > Best regards, > Benas > > On Thu, Jun 18, 2020, 7:02 PM Andreas Hennings <andr...@dqxtech.net> > wrote: > >> Hi >> >> The RFC introduces what I call a "meaningless choice", by making >> something possible that is currently illegal, but which does not change >> behavior. >> https://3v4l.org/daeEm >> >> It forces organisations, frameworks or the php-fig group to introduce yet >> another coding convention to decide whether or not there should be a ": >> void" declaration on constructors. >> >> I am ok with restricting the use of "return *;" inside a constructor. >> But I don't like allowing the ": void" declaration. >> >> Greetings >> >> On Thu, 18 Jun 2020 at 17:18, Benas IML <benas.molis....@gmail.com> >> wrote: >> >>> Hey Bob, >>> >>> Magic methods are **never** supposed to be called directly (even more if >>> that method is a constructor or a destructor). If that's not the case, >>> it's >>> just plain bad code. But by enforcing these rules, we make sure that less >>> of that (bad code) is written and as a result, we make PHP code less >>> bug-prone and easier to debug. That's also most likely the reason why >>> "ensure magic methods' signature" RFC opted in to validate `__clone` >>> method's signature and ensure that it has `void` return type. >>> >>> Just for the sake of making sure that you understand what I mean, here >>> are >>> a couple of examples that show that no magic method is ever supposed to >>> be >>> called directly: >>> ```php >>> // __toString >>> (string) $object; >>> >>> // __invoke >>> $object(); >>> >>> // __serialize >>> serialize($object); >>> ``` >>> >>> Moreover, by validating constructors/destructors and allowing an explicit >>> `void` return type declaration, we are becoming much more consistent >>> (something that PHP is striving for) with other magic methods (e. g. >>> `__clone`). >>> >>> Also, saying that "sometimes you have valid information to pass from the >>> parent class" is quite an overstatement. After analyzing most of the 95 >>> Composer packages that had a potential BC break, I found out that either >>> they wanted to return early (that is still possible to do using >>> `return;`) >>> or they added a `return something;` for no reason. Thus, no libraries >>> actually returned something useful and valid from a constructor (as they >>> shouldn't). >>> >>> Last but certainly not least, constructors have one and only one >>> responsibility - to initialize an object. Whether you read Wikipedia's or >>> PHP manual's definition, a constructor does just that. It initializes. >>> So, >>> the PHP manual is perfectly correct and documents the correct return type >>> that a constructor should have. >>> >>> Best regards, >>> Benas >>> >>> On Thu, Jun 18, 2020, 4:06 PM Bob Weinand <bobw...@hotmail.com> wrote: >>> >>> > > Am 17.06.2020 um 01:10 schrieb Benas IML <benas.molis....@gmail.com >>> >: >>> > > >>> > > Hey internals, >>> > > >>> > > This is a completely refined, follow-up RFC to my original RFC. >>> Based on >>> > the >>> > > feedback I have received, this PR implements full validation and >>> > implicitly >>> > > enforces `void` rules on constructors/destructors while also >>> allowing to >>> > > declare an **optional** explicit `void` return type. Note, that >>> there is >>> > a >>> > > small but justifiable BC break (as stated by the RFC). >>> > > >>> > > RFC: https://wiki.php.net/rfc/make_ctor_ret_void >>> > > >>> > > Best regards, >>> > > Benas Seliuginas >>> > >>> > Hey Benas, >>> > >>> > I do not see any particular benefit from that RFC. >>> > >>> > Regarding what the manual states - the manual is wrong there and thus >>> > should be fixed in the manual. This is not an argument for changing >>> engine >>> > behaviour. >>> > >>> > Sometimes a constructor (esp. of a parent class) or destructor may be >>> > called manually. Sometimes you have valid information to pass from the >>> > parent class. >>> > With your RFC an arbitrary restriction is introduced necessitating an >>> > extra method instead. >>> > >>> > In general that RFC feels like "uh, __construct and __destruct are >>> mostly >>> > void, so let's enforce it … because we can"? >>> > >>> > On these grounds and it being an additional (albeit mostly small) >>> > unnecessary BC break, I'm not in favor of that RFC. >>> > >>> > Bob >> >> On Thu, 18 Jun 2020 at 19:05, Benas IML <benas.molis....@gmail.com> wrote: > Hey Bob, > > Your examples (regarding constructors/destructors) do make sense since you > are using these methods for what they should be used: initialization and > freeing resources. But, this is off-topic to the RFC. > > This RFC only proposes to enforce no-return rule on > constructors/destructors. And based on my previous reply, top 2,000 > Composer packages that have a BC break don't actually return anything > (early returns and returns that don't change code behavior don't count). > > Your examples regarding `__toString`, `__invoke` and `__serialize` are also > not relevant to this RFC. Although, I do understand that you were > responding to email and I'm thankful for that :) > > > Best regards, > Benas > > On Thu, Jun 18, 2020, 7:43 PM Bob Weinand <bobw...@hotmail.com> wrote: > > > Hey, > > > > Am 18.06.2020 um 17:18 schrieb Benas IML <benas.molis....@gmail.com>: > > > > Hey Bob, > > > > Magic methods are **never** supposed to be called directly (even more if > > that method is a constructor or a destructor). If that's not the case, > it's > > just plain bad code. But by enforcing these rules, we make sure that less > > of that (bad code) is written and as a result, we make PHP code less > > bug-prone and easier to debug. That's also most likely the reason why > > > > > > __construct() is invoked directly on parent calls, sometimes to > > reinitialize an object or > > after ReflectionClass::newInstanceWithoutConstructor. > > > > I invoke __destruct() directly when needing an early freeing of existing > > resources. > > > > "ensure magic methods' signature" RFC opted in to validate `__clone` > > method's signature and ensure that it has `void` return type. > > > > Just for the sake of making sure that you understand what I mean, here > are > > a couple of examples that show that no magic method is ever supposed to > be > > called directly: > > ```php > > // __toString > > (string) $object; > > > > > > I like using ->__toString() in favor of (string) casts when the variable > > is guaranteed to be an object to highlight that and avoid magic-ness. > > > > // __invoke > > $object(); > > > > > > Same here, unless the object is a closure. > > > > // __serialize > > serialize($object); > > ``` > > > > > > Can't argue much about that one, I never use serialize(). > > > > Moreover, by validating constructors/destructors and allowing an explicit > > `void` return type declaration, we are becoming much more consistent > > (something that PHP is striving for) with other magic methods (e. g. > > `__clone`). > > > > > > Yeah, __clone() is odd. No idea why. > > > > Also, saying that "sometimes you have valid information to pass from the > > parent class" is quite an overstatement. After analyzing most of the 95 > > Composer packages that had a potential BC break, I found out that either > > they wanted to return early (that is still possible to do using > `return;`) > > or they added a `return something;` for no reason. Thus, no libraries > > actually returned something useful and valid from a constructor (as they > > shouldn't). > > > > Last but certainly not least, constructors have one and only one > > responsibility - to initialize an object. Whether you read Wikipedia's or > > PHP manual's definition, a constructor does just that. It initializes. > So, > > the PHP manual is perfectly correct and documents the correct return type > > that a constructor should have. > > > > > > It also is generally a bad idea to have side effects in constructors, but > > _sometimes_ it is justified. Only because something mostly is a bad idea, > > it is not always. > > Also note that other languages completely forbid manual ctor calls. But > > PHP doesn't (and for good reason, like after > > using ReflectionClass::newInstanceWithoutConstructor). > > > > Bob > > > > Best regards, > > Benas > > > > On Thu, Jun 18, 2020, 4:06 PM Bob Weinand <bobw...@hotmail.com> wrote: > > > >> > Am 17.06.2020 um 01:10 schrieb Benas IML <benas.molis....@gmail.com>: > >> > > >> > Hey internals, > >> > > >> > This is a completely refined, follow-up RFC to my original RFC. Based > >> on the > >> > feedback I have received, this PR implements full validation and > >> implicitly > >> > enforces `void` rules on constructors/destructors while also allowing > to > >> > declare an **optional** explicit `void` return type. Note, that there > >> is a > >> > small but justifiable BC break (as stated by the RFC). > >> > > >> > RFC: https://wiki.php.net/rfc/make_ctor_ret_void > >> > > >> > Best regards, > >> > Benas Seliuginas > >> > >> Hey Benas, > >> > >> I do not see any particular benefit from that RFC. > >> > >> Regarding what the manual states - the manual is wrong there and thus > >> should be fixed in the manual. This is not an argument for changing > engine > >> behaviour. > >> > >> Sometimes a constructor (esp. of a parent class) or destructor may be > >> called manually. Sometimes you have valid information to pass from the > >> parent class. > >> With your RFC an arbitrary restriction is introduced necessitating an > >> extra method instead. > >> > >> In general that RFC feels like "uh, __construct and __destruct are > mostly > >> void, so let's enforce it … because we can"? > >> > >> On these grounds and it being an additional (albeit mostly small) > >> unnecessary BC break, I'm not in favor of that RFC. > >> > >> Bob > > > > > > >
