Ruleset fully_qualified_strict_types is creating invalid code

[nickvergessen]

Talk integration tests have this file which imports TableNode:
https://github.com/nextcloud/spreed/blob/0d1fc13e36e40a6cd8dfa80915ea77f06b835f80/tests/integration/features/bootstrap/FeatureContext.php#L27

But the autofixing is breaking the docs by adding a leading \ because the file itself has no namespace

@@ -914,9 +914,9 @@
 	 * @param string $user
 	 * @param string $identifier
 	 * @param string $apiVersion
-	 * @param TableNode|null $formData
+	 * @param \TableNode|null $formData
 	 */
-	public function userCreatesRoom(string $user, string $identifier, string $apiVersion, TableNode $formData = null): void {
+	public function userCreatesRoom(string $user, string $identifier, string $apiVersion, ?TableNode $formData = null): void {
 		$this->userCreatesRoomWith($user, $identifier, 201, $apiVersion, $formData);
 	}
 

[come-nc]

We should remove fully_qualified_strict_types rule and report the bug upstream I think

[nickvergessen]

yeah let's do that and do a "quick" 1.2.1

[come-nc]

For the record, there are several issues and PRs opened regarding this rule, I think PHP-CS-Fixer/PHP-CS-Fixer#7719 would fix the issue you had.

But for now let’s just remove the rule to be able to update our coding-standard. We may try it again in some time.

[nickvergessen]

Let keep this open as reminder?

[Wirone]

IMHO you should keep fully_qualified_strict_types, but at the same time add no_superfluous_phpdoc_tags - there's no point in keeping these @params because all the information is already in the native signature. This method should look like:

    /**
	 * @Then /^user "([^"]*)" creates room "([^"]*)" \((v4)\)$/
	 */
	public function userCreatesRoom(string $user, string $identifier, string $apiVersion, ?TableNode $formData = null): void {
		$this->userCreatesRoomWith($user, $identifier, 201, $apiVersion, $formData);
	}

Then you don't need to wait for PHP-CS-Fixer/PHP-CS-Fixer#7719 and have less code in the repo 😉.

[come-nc]

IMHO you should keep fully_qualified_strict_types, but at the same time add no_superfluous_phpdoc_tags - there's no point in keeping these @params because all the information is already in the native signature.

We most probably have tags in places which are not superfluous and those would get messed-up by the rule as well. Also seeing several bug reports and open pull request against the same rule has made me scared to add it for now.

[Wirone]

We most probably have tags in places which are not superfluous and those would get messed-up by the rule as well. Also seeing several bug reports and open pull request against the same rule has made me scared to add it for now.

Only actually superfluous tags are removed (where type in phpDoc is exactly the same as in native signature + there's no additional description), so maybe just try how it works in practice 🙂.

In terms of fully_qualified_strict_types: this rule was massively developed lately, I've added feature for importing symbols found during analysis, we added several new places where FQCN can be found and shortened, there were many other improvements like relative imports. It's natural there were issues reported, but you can clearly see these were resolved - almost all remaining issues are feature requests, there is only one bug related to edge case, but even this issue has 2 opened PRs (with alternative implementation). There's no need to be scared, just look at how many test cases we have for it 🙂.