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

Introduce PHP 7 strict typing #9040

Open
szepeviktor opened this issue Jun 3, 2019 · 7 comments
Open

Introduce PHP 7 strict typing #9040

szepeviktor opened this issue Jun 3, 2019 · 7 comments

Comments

@szepeviktor
Copy link
Contributor

It raises overall code quality.
https://www.php.net/manual/en/control-structures.declare.php

Please consider adding declare(strict_types=1); to every PHP file.

@robbieaverill
Copy link
Contributor

We've agreed to introduce PHP 7.1 as the minimum requirement for SilverStripe 4.5 onwards (from the 4 branch at time of writing) and PHP 7.2 (at time of writing) in SilverStripe 5.x in the master branch.

With SilverStripe 5.x changes we can break APIs, but in the meantime we'll need to keep APIs semver compliant in 4.x releases. For this reason, yes we can add strict typing for new APIs we write, but I don't think we can add it to existing code in the 4.x line. In 5.x we can probably start doing it in bulk.

Is this something you'd like to contribute to? If not, we could chalk it up to a development practice and close the issue for now.

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jun 4, 2019

It is very easy:

composer require slevomat/coding-standard
vendor/bin/phpcbf --standard=vendor/slevomat/coding-standard/SlevomatCodingStandard --sniffs=SlevomatCodingStandard.TypeHints.DeclareStrictTypes src/ tests/

May I send a PR?

@robbieaverill
Copy link
Contributor

Yes please, into the master branch :-)

@szepeviktor
Copy link
Contributor Author

Done.

You could add the above phpcs sniff to sustain strict typing.

@sminnee
Copy link
Member

sminnee commented Jun 30, 2019

Hang on. I would note that declare(strict_types) isn't an API breakage. Making our APIs return consistent types would be, and it might simplify the creation of code to comply with strict_typing, but in some ways I'd prefer a partial application of strict_types in 4 that we polish in 5, rather than a massive divergence between 4 and 5 on this point.

@robbieaverill
Copy link
Contributor

We can already add it to new APIs in SS4

@sminnee
Copy link
Member

sminnee commented Jun 30, 2019

Note that declare(strict_types) applies to methods being called, not methods being defined.

The latter is the domain of type definitions on method signatures.

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

Successfully merging a pull request may close this issue.

4 participants