issue #118 - optional typed php 7.4 properties are raising engine errors

[pounard]

Fixes #118.

[pounard]

A few details are questionable:

• addition of the ObjectProperty class: the code could have been less verbose, but it moves out a few responsibilities outside of the generator class,

• same ObjectProperty class, I usually don't write the constructor when I use static factory methods, but anyway, I would have written an empty static constructor otherwise, it's just a matter of taste,

• rewrite of code and factorisation into generatePropertyHydrateCall() that's subjective, I just find this variant more readable.

[pounard]

Psalm is quite aggressive or I am too lazy ?

[Ocramius]

Psalm is quite aggressive or I am too lazy ?

It is very aggressive, which is good, since every tiny detail has major implications in OSS packages, even niche ones like this one (which BTW has ~500 installs/day)

[pounard]

It is very aggressive, which is good

Yes, it was sarcasm. It's actually quite drastic, I'll fix most errors I can.

[Ocramius]

I'll fix most errors I can.

Please only fix what's raised by your patch: I can work on the rest myself, while merging

[pounard]

Please only fix what's raised by your patch

Yes of course.

[pounard]

I have to admit that fixing those kind of errors from phpcs:

• Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space
• Expected 1 space(s) after NOT operator; 0 found

is quite annoying, are those part of your conventions for this package ?

[pounard]

Same question for "Variable "$propertyName" not allowed in double quoted string; use sprintf() or concatenation instead", what's your preference between:

• "foo " . $bar . " bar",
• "foo {$bar} bar",
• \sprintf("foo %s bar", $bar)

I mostly use the \sprintf() variant personally, except when generating code in a template fashion, where I prefer the "{$bar}" variant (it avoids having multiple dangling ' or " in the same line, which makes the whole more readable, once again, it's probably a matter of taste), especially when using the heredoc syntax, but I'll comply to your preference.

[Ocramius]

The first and the last are acceptable, string interpolation is to be avoided.

Don't overthink it BTW: if you need to auto-fix things, there's vendor/bin/phpcbf

[Ocramius]

are those part of your conventions for this package ?

Yes, this package relies on doctrine/coding-standard

[pounard]

The first and the last are acceptable, string interpolation is to be avoided.

Is there a technical reason for this ? From a code reader perspective, it seems much more fluent to read.

[pounard]

Don't overthink it BTW: if you need to auto-fix things, there's vendor/bin/phpcbf

OK, I think it's your job then, seeing the commit history :) For the rest, if there's anything more structural or important to change, please tell me.

[Ocramius]

The first and the last are acceptable, string interpolation is to be avoided.

Is there a technical reason for this ? From a code reader perspective, it seems much more fluent to read.

Yes, I much prefer to avoid variable interpolation in strings: explicit concatenation is preferable.

I'll review again and take over for merge, if there's nothing left 👍

[pounard]

Thanks a lot !

[pounard]

Yes, I much prefer to avoid variable interpolation in strings: explicit concatenation is preferable.

OK, I see that the technical reason is mostly taste :) If that's auto-fixable using phpcbf I'll leave it to you then !

[pounard]

What's the status of this PR ? Should I fix any other things ?

[Ocramius]

Didn't get to look at it, sorry

[pounard]

Don't worry, I'm just asking, we are actually starting a PHP 7.4 project where I'd like to use this component. We can still patch it locally but I'd very much prefer to use a vanilla stable version :)

[pounard]

Any news on this ? @Ocramius if you anything bothers you on how I did it I would pleased to fix or do another MR.

[Ocramius]

@pounard I'm checking this out locally: sorry, it simply slipped out of my TODOs. Thanks for the ping!

[Ocramius]

@pounard I've bumped to PHP 7.4, brought mutation test coverage back to 100% and applied CS fixes on top of your patch.

Turns out that verifying if ($property->allowsNull && ! $property->hasDefault) { was sufficient for our purposes! 👍

Thanks!

[pounard]

Thank you !