Define a standard for adding declare(strict_types=1)

I agree this is a good idea, I'd love to follow PSR-12 here, since we seem to already be doing a lot of it?

While I personally like the variant better where declare() comes before any @file doc block (not on the same line as <?php, though), I’m also in favor of just following PSR-12 here. (This is also getting less and less relevant, with most of our code now in classes without @file doc.)

However, it seems there is an error in the IS? PSR-12 is option A, right, not option B?

Coming here from #3376057: [META] Add declare(strict_types=1) to all tests. We're trying to move forward with strict typing in core, but are nearly blocked on deciding the standard for this. Core is already inconsistent.

At #3401994-13: Add declare(strict_types=1) to all test traits @xjm is in favor of spaces:

I did notice one thing regarding the coding standard of the declaration itself. I also belatedly notice that this is a point in the remaining tasks section as well. Per our coding standards for comparison and assignment operators, the space is required on either side of the strict type declaration equals sign. If and when that's properly enforced (or properly enforced within a function call), we might end up with a situation where the two CS rules conflict with each other. So, I think we should use the format with the spaces.

At first I agreed, but now I'm back on the fence and open to the PSR-12 argument that this shouldn't use spaces. /shrug

Anyway, we need to decide, ideally soon. 😅

If it means anything, a Github code search for "declare(strict_types=1)" returns 2.9M results, whereas "declare(strict_types = 1)" returns only 247k results.

Yes I think the issue summary is wrong?

It would be good to have a variant example of option A where there's no file level docblock, which is the case for nearly all of our classes now. I think that's what everyone uses in practice already.

Updated IS added example.

I think a critical factor would be what our standards for named arguments are (is there an issue for that yet?). I'd always assumed named arguments would be equivalent to method declarations and argument default values, with the space on either side of the equals sign. My primary concern is having two CS rules that disagree, and if we avoid that, also making it harder to remember because we have two similar standards that disagree.

Places Drupal already requires the space:

1. On either side of a comparison operator.
2. On either side of an assignment operator.
3. On either side of a default value operator in a function declaration.
4. Basically between anything and anything else, everywhere.

If we wanted to adopt more of PSR-12, that would be a far more massive undertaking (it has a cuddled else and all that). That said, it does appear PSR-12 uses spaces around comparison, assignment, and default value operators, but not in the specific case of a named argument for strict types, at least. That seems inconsistent, but if the standard can enforce those things without conflicts, I'm not too fussed if Drupal does or doesn't either way.

-1 on having anything on the same line as the <?php tag; that way lies heartache. I don't care where anything is relative to the file docblock really.

My personal opinion is to follow PSR-12 (and largest part of the community) and put it on its own line.

Also as you mentioned in the linked issue, it is not a declaration. Another reason you could argue for no spaces is the error you get if you enforce strict types with phpcs:

composer phpcs -- --report-full --report-summary --report-\\Micheh\\PhpCodeSniffer\\Report\\Gitlab=phpcs-quality-report.json
> phpcs --standard=core/phpcs.xml.dist --parallel=$(nproc) -- '--report-full' '--report-summary' '--report-\Micheh\PhpCodeSniffer\Report\Gitlab=phpcs-quality-report.json'
FILE: ...ests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] Missing declare(strict_types=1).
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
Time: 9.74 secs; Memory: 6MB
PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
...ts/Core/Database/SchemaIntrospectionTestTrait.php  1       0
----------------------------------------------------------------------
A TOTAL OF 1 ERROR AND 0 WARNINGS WERE FOUND IN 1 FILE
----------------------------------------------------------------------
PHPCBF CAN FIX 1 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

The suggestion is without spaces. phpcbf will probably fix without spaces also. This will get confusing when checks are introduced and probably lead to more problems and extra feedback loops.

posted same time as xjm's post :x

Issue for names arguments: #3337834: Standards for PHP named arguments

I see this as a test of the coding standards process.

What matters is that we can get this through the process in as short as possible time. If we can't then the process needs to be more agile.

Update IS to codestyle issue template

Added myself as supporter

The linked page was wrong in the is.

@xjm, not sure named arguments matter here much. Other pattern, other usage. Don't really see the direct relation.

Happy to lend my support to this. I went back and forth a little, but I think there's value in aligning on PSR-12, as it will already be familiar to those new to Drupal and existing Drupal developers.

Proposal has received three supporters, change record exists, I believe this can now go to RTBC and committee stage (based on the process).

@bbrala As @mstrelan documented, the rule can be configured either way and PHPCBF will fix things accordingly, so that's not a reason in either direction.

For me the most compelling point to not include the spaces is about the sheer magnitude of preference for the no-space version elsewhere on the internet.

The summary was still confusing what PSR-12 says with the two stated options. It seems that everything is aligning towards Option A. That's my understanding of the PSR-12 specs, and from my grep'ing, that's what core implementations are already doing (other than the inconsistency regarding spaces around the = sign.

Added myself as another supporter of option A, and made it explicit that the listed supporters are for option A. Also, fixed the date formatting on the supporters. https://xkcd.com/1179 😂

Moving to RTBC so that the standards committee can review. Even though most (all?) of the sponsors are on the committee, this is the formal process we're supposed to take. 😅

Onwards!
-Derek

@bbrala As @mstrelan documented, the rule can be configured either way and PHPCBF will fix things accordingly, so that's not a reason in either direction.

Sorry, I missed that, I dove into the rules. :)

Named arguments are currently discouraged in core - i.e. we more or less tell people not to use them / be prepared for breakage if they do #3183180: [policy, no patch] Document named arguments BC policy.

Fixed the Change Request link in IS checklist. The [# ] syntax did not create an active link.

This was discussed at the coding standards committee meeting and it was agreed that we should proceed to the next step.

The consensus was for no spaces in alignment with PSR 12.

Tagging for announcement.

Updated the remaining tasks in the Issue summary. The new process doesn't include the "Needs announcement for final discussion" but it is also not doing any harm so leaving that for now.

The issue for creating the post is #3403582: [2023-11-29] Draft blog

https://www.drupal.org/about/core/blog/coding-standards-proposals-for-fi... is now out, removing the tag for that.

This does impact core, so I can't immediately add "Needs documentation updates". But I'm not sure what to tag it with, instead. There is not yet a "Needs core committer approval". Is "Needs committer feedback" sufficient? Also explicitly tagging for release manager and framework manager review for now, but let's figure out exactly how we want this to work and update the project page accordingly.

Thanks,
-Derek

I propose the new section should be added right after the section on PHP Code Tags. I already put that in the summary, but please comment if there are any objections.

Thanks,
-Derek

Approving from a FM POV

I am against option A. It would add two extra lines to each PHP file. The option B (I call it PhpUnit style) seems to me more readable.
Also all code generated by `drush generate` use option B (with spaces around 1).

I think option A is the most widely used and the only one that will allow for php having another declare statement in the future in some kind of elegant way.
So I agree with the proposed resolution of accepting this into our standards.

I agree with @borisson_ on using Option A, putting declare on a separate line. Option B would create a mess if you would use multiple declare statements (in the future), which are part of PHP syntax. So besides a coding standard for just "declare(strict_types=1);" It would be better to look at the global picture and define a coding standard for using "declare" statements al together. (https://www.php.net/manual/en/control-structures.declare.php)

Approving for RM too.

This was discussed at core committers meeting on Wed December 5th (UTC)
There were no objections

Completing steps 6 and 7.

Next step is the documentation edits and core issue for phpcs rule.

We're already working on a rule in the issues to add strict typing to tests, so that followup will not require any additional work beyond gradually expanding what parts of the codebase the rule applies to.

Thanks everyone!

I completed the documentation steps. I did change the change records, only to add a link to the documentation page.

According to #34 , this still needs a "core issue for phpcs rule".

The "core issue for phpcs rule" is a bit tricky because we can't just enable it everywhere without breaking stuff. We have #3376057: [META] Add declare(strict_types=1) to all tests which is gradually adding it with various glob patterns in child issues, but perhaps need to add something to #3050720: [Meta] Implement strict typing in existing code for enabling it in runtime code.

@mstrlen, thanks for the reminder.

I added #3407820: [Meta] Fix 'SlevomatCodingStandard.TypeHints.DeclareStrictTypes' coding standard which is postponed on #3050720: [Meta] Implement strict typing in existing code. That seems like the best step to take for now.

I think everything is completed here and this can be closed now.

Really great to see this. Just as a side note, in the past we've always aimed to rewrite any PSR-inspired rules to avoid strong all-caps wording such as "MUST" because it doesn't really fit in our coding standards style which is more friendly and conversational than the style used in PSR.

In our "friendly" style the proposed wording:

If you define strict types for a PHP file, the declare statement MUST be on a new line after the opening PHP tag surrounded by empty newlines. If the PHP file has a file level docblock the declare statement MUST be positioned after that. The declare statement MUST be written without spaces around the equals sign.

could be something like:

If you define strict types for a PHP file, place the declare statement on a new line after the opening PHP tag surrounded by empty newlines. If the PHP file has a file level docblock the declare statement should be positioned after that. The declare statement is written without spaces around the equals sign.

I think pfrenssen's style is an improvement and we should make the chnages suggested in #39. It has exactly the same technical outcome, but is written, as Pieter says, in a friendly style. I know this issue has already been through the final steps of approval, but if this "conversational" style is how Drupal standards should be written then we need to fix the recently updated page. Also we should check for this style of writing during the review process, so that we avoid late fixes like this next time.

The 'needs documentation updates' tag was already still there.

+1 to the "conversational" text in #39, and in us keeping an eye out for this style of writing for future issues.

I don't believe this needs another round of formal approval. The standard has already been approved and decided. We're only wordsmithing the docs on how to explain the standard. But I do think we need a few other folks to sign-off so it's not just a single person making edits. If a couple more of the active CS team members +1, let's make the edit.

Thanks!
-Derek

+1 to the friendly text in #39.

@pfrenssen, thank you providing the background and improving the text!

There is agreement here for changing the text to be friendly Drupal style and it does make this consistent with the rest of the coding standards. Therefor, I have made the change. And I updated the IS with the new paragraph.

This made me recall seeing #1795750: Revise coding standards to use IETF RFC 2119 standards so I added that as a related issue.

I think this can be set to 'Fixed' now.

Thanks for updating the text!

This is done, I'm marking as fixed. Thanks to all involved for getting this in!

I think pfrenssen accidentally submitted with stale form data. The issue summary updates from #43 have been reverted. I can fix it.

I have put back pfrenssen's unintended change from #44. This should match #43 now.

Fixed missing 'if'
Also updated the Change Request with the same new text.

I have updated the remaining tasks and removed the last tag.

Thanks to @pfrenssen and @jonathan1055 for the prompt follow up.

Added links to the Coder issue and Core issue.

The Coder issue #3407995: Add sniff for declare(strict_types=1); has been committed and this sniff will be available in Coder 8.3.23 (or whatever the next release is).

The current Coder release is 8.3.22 from Oct 2023, which is the version being used in contrib testing on Gitlab pipelines using core 10.1 and 10.2, and also DrupalCI 10.2

Amending attribution.