Add strict_types, type hints and phpcs comments to generated code templates#418
Conversation
jensschuppe
force-pushed
the
generated-code-style
branch
from
ad91236 to
5fac58f
Compare
| // phpcs:disable PSR1.Files.SideEffects | ||
| require_once '<?php echo $mainFile ?>.civix.php'; | ||
| // phpcs:enable |
There was a problem hiding this comment.
What's the purpose of these phpcs comments? civix already writes code that passes all the checks run by the civilint command. Anyone using civilint should be fine without this?
There was a problem hiding this comment.
The phpcs configuration in systopia/civicrm-extension-template is stricter than civilint and adds this sniff. Since each extension uses this pattern (while not encouraged elsewhere), we find ourselves adding these comments in every extension after civix built the file.
So technically, you're right, but I'd say these comments don't hurt … 🤷
| ?> | ||
|
|
||
| declare(strict_types = 1); | ||
| <?php |
There was a problem hiding this comment.
This formatting seems unnecessarily messy. Why not keep the php code together instead of splitting it? E.g. the echo and the $_namespace can all stay within the same code block so we don't need so many extra opening and closing php tags.
There was a problem hiding this comment.
Agree, this was a quick shot during the sprint and should be simplified.
| * @link https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_config/ | ||
| */ | ||
| function <?php echo $mainFile ?>_civicrm_config(&$config): void { | ||
| function <?php echo $mainFile ?>_civicrm_config(\CRM_Core_Config $config): void { |
There was a problem hiding this comment.
Tangential: It's weird that the hook defines $config as pass-by-reference. The $config is so big and central... it's hard for me to imagine what happens if one replaces that object. (My gut says that part of the of hook signature is leftover from the PHP4/PHP5 era, with the rationale lost to the sands of time...)
In any case, it's probably good that this makes it look like a regular object-param. Inviting people to think of it as a reference is... inviting trouble (AFAICS, for no real reason)...
Aside: We should also add the newer ?array $flags = NULL because anyone who actually uses this hook probably should use $flags. (But that's can be a separate issue.)
totten
force-pushed
the
generated-code-style
branch
from
5fac58f to
4bb7b7e
Compare
| @@ -1,10 +1,11 @@ | |||
| <?php | |||
| echo "<?php\n"; | |||
| echo "declare(strict_types = 1);\n"; | |||
There was a problem hiding this comment.
Ok. JFYI this is the opposite of how I'd have done it. IMO the first file in the diff is cleanest: just put the line in the template, no need for echo.
There was a problem hiding this comment.
Yeah, I can see that being a bit shorter. (But 1 SLOC either way.) They're both an improvement.
(N.B. Some files have dynamically-computed parts -- like namespace Foo; or use Foo as E; -- which come immediately after the declare(). When you alternate PHP and plain text, newlines get weirder. This formulation was easy to copy-paste into many places. 🤷 )
|
Tangential notes, for posterity:
|
4 participants
This adds strict_types declarations, type hints and PHP Code Sniffer ignore comments and
finalclass keywords to relevant generated PHP code that could be subject to QA tool inspections.Since we're trying to implement https://github.com/systopia/civicrm-extension-template for all of our extensions, this saves a lot of repetitive work.
Ping @dontub for an eye on that.
For entity type definition files this will need a follow-up to totten/php-array-doc.