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.

Sign up for GitHub

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

Jump to bottom

[New Rule] Add declare(strict_types=1) to your PHP files #33

Open
lenaorobei opened this issue Feb 1, 2019 · 11 comments · May be fixed by #488
Open

[New Rule] Add declare(strict_types=1) to your PHP files #33

lenaorobei opened this issue Feb 1, 2019 · 11 comments · May be fixed by #488
Assignees
@fredden
Labels
need to discuss Rule requires discussion new rule New feature implementation Progress: PR created proposal New rule proposal technical guidelines The rule is based on Magento Technical Guidelines version specifiс Rule is applicable to specific Magento version only waiting for feedback Additional explanation needed

Comments

@lenaorobei
Copy link
Contributor

Rule

All new PHP files MUST have strict type mode enabled by starting with declare(strict_types=1);. All updated PHP files SHOULD have strict type mode enabled. PHP interfaces SHOULD NOT have this declaration.

Reason

Source: Magento Technical Guidelines.

With PHP 7, it is possible to add type hinting to your code. However, this doesn't mean that types are actually enforced, unless strict typing is enabled by adding declare(strict_types=1) to the top of each PHP file.

PHP code becomes more robust when type hinting (argument types, return types) are added. With the declare(strict_types=1) added, there is less chance for bugs that related to type casting.

Implementation

Please refer to ExtDN an generic PHP CodeSniffer (not merged yet) implementations.

Important details

This sniff should be applicable to Magento >=2.2, because older versions of Magento supports PHP 5.5 and 5.6.
Follow the discussion about version specific sniffs for more details.
@lenaorobei lenaorobei added proposal New rule proposal need to discuss Rule requires discussion new rule New feature implementation technical guidelines The rule is based on Magento Technical Guidelines labels Feb 1, 2019
@larsroettig
Copy link
Member

larsroettig commented Feb 1, 2019 •
edited
Loading

@lenaorobei I can implement this

@lenaorobei
Copy link
Contributor Author

@larsroettig sounds good, but first we need to discuss the approach for version specific sniffs.

@lenaorobei lenaorobei added the version specifiс Rule is applicable to specific Magento version only label Feb 6, 2019
This was referenced Feb 6, 2019
Closed
Closed
@buskamuza
Copy link

When implementing, please remove a static test that cover the same thing.

@lenaorobei lenaorobei added the waiting for feedback Additional explanation needed label Feb 13, 2019
@lenaorobei
Copy link
Contributor Author

As per architects discussion we need to determine how strict this rule should be:

  • Applicable only to new files?
  • Or only to changed files?
  • How to check vendors extensions?

@sergeynezbritskiy
Copy link
Contributor

sergeynezbritskiy commented Apr 13, 2019 •
edited
Loading

There is a good sniff which checks that your code satisfies php7 styling code (type hints, return types, declaring strict type etc)
https://github.com/slevomat/coding-standard, can we reuse it ?

@fooman fooman mentioned this issue Jan 8, 2020
Closed
@tmotyl
Copy link

tmotyl commented Jan 9, 2020

Any progress here? Is there help needed?

@lenaorobei
Copy link
Contributor Author

@tmotyl
This coding standard is used to check older Magento 2.x versions that are compatible with PHP < 7.0. This is the reason why this rule is not beeng added here. We support only one version of coding standard, so there is no specific branches for separate Magento releases.

@markshust
Copy link

Since PHP 8 is now released, is it time to drop support for PHP < 7.0 and merge this in?

magento-devops-reposync-svc pushed a commit that referenced this issue Aug 30, 2021
@sivaschenko
Merge pull request #33 from magento-commerce/imported-magento-magento…
e83a012 
…-coding-standard-236

[Imported] AC-940: Create unit test for Magento2\Less\BracesFormattingSniff check
@fredden

This comment was marked as outdated.

Sign in to view
@guvra
Copy link

guvra commented Aug 25, 2022

It looks like this can be closed. PHP version 8 has this feature enabled and does not require the extra line of code in each file.

This is not true.
You are probably thinking of this.
This has nothing to do with strict types.

@fredden
Copy link
Member

fredden commented Aug 25, 2022

This is not true.

Sorry, I was wrong. I've marked my comment as hidden to avoid confusion going forward. Thanks for calling this out.

@fredden fredden linked a pull request Sep 27, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fredden fredden

Labels
need to discuss Rule requires discussion new rule New feature implementation Progress: PR created proposal New rule proposal technical guidelines The rule is based on Magento Technical Guidelines version specifiс Rule is applicable to specific Magento version only waiting for feedback Additional explanation needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Require a strict_types declaration fredden/magento-coding-standard
8 participants
@fredden @markshust @tmotyl @buskamuza @guvra @larsroettig @sergeynezbritskiy @lenaorobei