Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VOTE] Use & Enforce strict type declaration #36

Open
ronan-gloo opened this issue May 7, 2021 · 5 comments
Open

[VOTE] Use & Enforce strict type declaration #36

ronan-gloo opened this issue May 7, 2021 · 5 comments
Labels

Comments

@ronan-gloo
Copy link
Contributor

ronan-gloo commented May 7, 2021

Summary

Lets quote the php.net documentation

By default, PHP will coerce values of the wrong type into the expected scalar type declaration if possible. For example, a function that is given an int for a parameter that expects a string will get a variable of type string.

It is possible to enable strict mode on a per-file basis. In strict mode, only a value corresponding exactly to the type declaration will be accepted, otherwise a TypeError will be thrown. The only exception to this rule is that an int value will pass a float type declaration.

Here i propose to enforce declare(strict_types=1); declaration for every php file under phpcs inspection in projects.
Details about type coercision are explained in the php doc linked below and article at the end of this description.

But here a short example about changes

Code Example

<?php
declare(strict_types=1);

function sum(int $a, int $b): int 
{
    return $a + $b;
}

# Valid
sum(3, 3);

# Invalid
sum(3, '3'); // Uncaught TypeError: add(): Argument #2 ($b) must be of type int, string given
sum(3, 3.0);  // Uncaught TypeError: add(): Argument #2 ($b) must be of type int, float given
sum(3, true);  // Uncaught TypeError: add(): Argument #2 ($b) must be of type int, bool given

Migration

The PHPCS rule is shipped with an autofix code that inject the declaration in files (tested and thats ok).
Beside that, many PHPUnit test are broken after the upgrade because of non strict typing, and it will need to spend some time to review and fix (any volunteers ? 😆 )

I'm convinced it worse the try because strict type may help us to write more consistent code and catch bugs early.
It's also a good dropin replacement for static analyzed types by PHPStan.

Rule declaration

 <rule ref="SlevomatCodingStandard.TypeHints.DeclareStrictTypes"/>

Resources

Article where the code example came from: https://barryvanveen.nl/blog/50-enforce-strict-type-checking-in-php-with-php-code-sniffer

The rule documentation: https://github.com/slevomat/coding-standard#slevomatcodingstandardtypehintsdeclarestricttypes-

@mdumoulin
Copy link
Contributor

Ok sur le principe, mais ça fait un gros changement et je me demande ce que ça va donner en pratique. Par exemple, je suis clairement convaincu que c'est bien côté domaine, mais il y a des endroits où la conversion automatique peut-être confortable. Par exemple quand on instancie un Model à partir de données récupérées via PDO.

Je ne vois pas trop comment on ferait techniquement, mais peut-être qu'une bascule en plusieurs étapes permettrait de conforter ce choix.

@ronan-gloo
Copy link
Contributor Author

En théorie tu n'auras ni plus ni moins à fixer ce que l'on trouve dans la baseline de PHPStan.
Par exemple : https://github.com/shoppingflux/api/blob/0ff3d1dd3c78132e4a4052e6b89d901a27aab536/phpstan-baseline.neon#L30-L32

@ddattee
Copy link
Contributor

ddattee commented May 10, 2021

Je suis ok aussi .
Coté channel connector ca devrait pas poser trop de probleme.
Par contre coté legacy on reflechiera a 3x (on réfléchit deja a 2x :p ) avant de typer quoique ce soit mais ca me semble être pour le mieux quand même

@ronan-gloo
Copy link
Contributor Author

Oups 🥁

@budrea
Copy link
Contributor

budrea commented May 10, 2021

Sauf erreur de ma part, DBAL te sort les résultats en string, peu importe leurs types. Du coup, pour que la conversion des données sorties de la BDD en Modèles de lecture marche avec declare(strict_types=1);, faut faire beaucoup plus que

fixer ce que l'on trouve dans la baseline de PHPStan.

Je viens de tester sur Catalog. Faut caster un peu partout. C'est faisable, je ne dis pas le contraire, mais je +1 la possibilité d'aller module par module / étape par étape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants