Add declare(strict_types=1) everywhere

We went all in on strict types at work and there's a grace period where you end up with a few TypeErrors at run time while you iron out stuff. And we already had phpstan at level 7 which is something core doesn't have the luxury of.

Just saying we need to be cautious here

Duplicate of 🌱 [policy, no patch] Use declare(strict_types=1) in new code Active ?

From that issue, Symfony rejected the same idea: https://github.com/symfony/symfony/issues/28423

hmm 🌱 [policy, no patch] Use declare(strict_types=1) in new code Active was supposed to be for new code, but ended up discussing adding it to all existing code too. I think we might want to keep that as a policy issue (for eventual phpcs rule etc.) and move implementation over here?

https://externals.io/message/112230#112233 shows that the Symfony decision might be getting outdated, but it's going to depend on the next few PHP 8 and eventually 9 releases as to what happens. At the moment we hit new type deprecation errors every PHP 8 minor, some of which aren't caught by tests, however it's probably better that they're deprecation notices than fatal errors.

Maybe we could start with adding strict types to every test file? That would have caught my silly issue, and it's guaranteed not to break production sites.

+1 to adding them to all tests. Should we repurpose this issue for that and bring it under the existing meta?

Yep let's do that.

We can use \Rector\TypeDeclaration\Rector\StmtsAwareInterface\DeclareStrictTypesRector for this.

rector.php:

<?php

use Rector\Config\RectorConfig;
use Rector\TypeDeclaration\Rector\StmtsAwareInterface\DeclareStrictTypesRector;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->rule(DeclareStrictTypesRector::class);
};

composer require rector/rector --dev
find core -type d -name "tests" -exec php ./vendor/bin/rector process {} \;

We may want to adjust the pattern as it includes test modules, fixtures etc which may or may not be in scope.

Thanks for automating this and opening the MR! Moving to NR, although beware -- the GitLab tab to review the diff says: "Changes 1000+" 😂

Oh hah, the GitLab pipeline is having all sorts of trouble trying to validate this MR, e.g, the cspell job dies thusly:

/scripts-124335-251871/step_script: line 256: /usr/bin/tr: Argument list too long
/scripts-124335-251871/step_script: line 256: /usr/bin/yarn: Argument list too long

Not sure where to go from here. I imagine MRs touching this many files are rare enough that we don't want to go down the rabbit hole of trying to fix our tooling to handle this kind of change set.

However, I doubt we want to split this up into N different MRs so each one will actually pass.

Would love some feedback from a framework manager (or release manager) on how to proceed. Tagging for framework manager for now.

Thanks/sorry,
-Derek

I've disabled spellcheck for now and manually revert files that phpcs complained about (adding declare before @file). We now have 78 tests to fix up!

I misread the output, there were 78 tests that failed but around 3000 errors. This seems like it might need splitting up in to much smaller portions.

Given unit tests alone require code fixes to 26 files I think we can split that off to a child issue, then determine what makes sense for remaining tests. I've opened 📌 Add declare(strict_types=1) to all unit tests Active for this.

Yeah makes sense to split off any actual code changes then see how we're looking after that.

There's existing rector rule so the patch becomes reproducible and CR can give contrib maintainers a guide to follow

@andypost yes it's described in #8. See related issue where unit tests are all fixed.

I mean Gitlab CI needs a rector's run with subset of dirs (which already converted) to prevent regressions

This way will allow maintain a list of tests/files to convert or already converted and prevent regressions same time
Also it will allow to sort out tests which needs code changes to apply the rector

@andypost I'm not sure about adding rector to GitLab CI. We can use the existing phpcs job with \SlevomatCodingStandard\Sniffs\TypeHints\DeclareStrictTypesSniffbut again, not sure about limiting that to just tests.

@catch do you mean the child issues should only make the code fixes and we leave adding declare(strict_types=1) to this parent issue? That would definitely make it easier to review those child issues.

mstrelan → closed merge request !5195

Changing to a meta, adding child issues to IS.

Added 📌 Enforce strict types in tests Fixed with phpcs config

Per @xjm in #3401994-13: Add declare(strict_types = 1) to all test traits → we're going with spaces around the =

declare(strict_types = 1);

Documenting that here and updating the title and summary accordingly.

Per @xjm in #3401994-13: Add declare(strict_types = 1) to all test traits we're going with spaces around the =

declare(strict_types = 1);

Is that a decision for the coding standards committee? A change like that impacts more than core.

Probably, yes, says one of the new members of that committee. 😂

We can still proceed with fixing strict type errors and such, but yeah, we probably shouldn't proceed with #3401994 until that's formally decided.

And lo, such an issue already exists: #3303206: Define a standard for adding declare(strict_types=1) → . Adding that as a new phase to the summary.

Belated "oopsie" as the committed changes should have been tagged for release notes due to the impact on tests and the new phpcs rules (it is my fault for not catching this). We'll need a similar note again in 10.3 for the rest of the conversions, so adding it here in a minute..

And I guess a lightweight CR for that matter.

Added the draft snippet. It can be iterated on when we create the CR and when we finish all the conversions for 10.3.

It looks like the child issues are not complete yet, so let's move this to the next pair of minor releases.

Down to the last issue 📌 Enforce strict types in tests Fixed . Added a CR.

I was looking at issues tagged for some manager review. 10 months ago this was tagged, 'Needs framework manager review' for problem with the size of the MR. In the interim that has been resolved using child issues to scope the work. As @mstrelan points out in #38, this is now down to the last issue so I think it is safe to remove the tag.

All the child issues are complete. Marking NR rather than Fixed as there are release notes and a change record to be finalised.

Reading the notes and CR they're pretty straight forward.

I moved the CR from here to the issue that enabled the change in phpcs.xml.dist and published it. I updated the release note snippet.

Thanks everyone for completing this in under 1 year!

Automatically closed - issue fixed for 2 weeks with no activity.

This wasn't completed for 10.4.0.