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

[New Rule] Static methods SHOULD NOT be used #24

Closed
lenaorobei opened this issue Jan 30, 2019 · 6 comments
Closed

[New Rule] Static methods SHOULD NOT be used #24

lenaorobei opened this issue Jan 30, 2019 · 6 comments
Assignees
Labels
accepted New rule is accepted new rule New feature implementation Progress: good first issue Issues is easy to get started with technical guidelines The rule is based on Magento Technical Guidelines

Comments

@lenaorobei
Copy link
Contributor

lenaorobei commented Jan 30, 2019

Rule

The use of static methods is discouraged.

Reason

Source: Magento Technical Guidelines.

Plugins cannot be used with static methods, because static methods cannot be intercepted.
As a result, It hurts extensibility.

Implementation

  • Subscribe to the T_STATIC token.
  • If T_FUNCTION is found right to it raise a warning.
@lenaorobei lenaorobei added proposal New rule proposal new rule New feature implementation technical guidelines The rule is based on Magento Technical Guidelines labels Jan 30, 2019
@paliarush paliarush added accepted New rule is accepted and removed proposal New rule proposal labels Jan 30, 2019
@lenaorobei lenaorobei added the Progress: good first issue Issues is easy to get started with label Jan 30, 2019
@larsroettig
Copy link
Member

Hi i will work on this at next weekend

@larsroettig larsroettig self-assigned this Feb 7, 2019
@YevSent
Copy link

YevSent commented Feb 13, 2019

This rule should not include tests which use PHPUnit because all assert..., once(), atLeast(), etc. are static methods.

@paliarush
Copy link
Contributor

@joni-jones I believe this is not relevant because the proposed sniff is looking for declaration of the static tests, not usage. Additionally, in the recent versions of PHPUnit all this methods are not invoked statically.

@YevSent
Copy link

YevSent commented Feb 27, 2019

Additionally, in the recent versions of PHPUnit all this methods are not invoked statically.

This issue contains the explanation sebastianbergmann/phpunit#1914 and you can see Sebastian's answer and the community reaction.

lenaorobei added a commit that referenced this issue Feb 27, 2019
#24: [New Rule] Static methods SHOULD NOT be used
@lenaorobei
Copy link
Contributor Author

Implemented in #24

@hopeseekr
Copy link

The PHPUnit maintainer is simply wrong on this issue. I have thoroughly analyzed PHPUnit's code, and unless we are going to declare willfully disregarding the static method calls are OK, then such an important code quality tool as PHPUnit should have code written for it using self::assertEquals() vs. the contextually ambiguous $this->assertEquals().

Or, PHPUnit needs to have a third class that does something like

// Expose static assert methods for people who need them.
class Assert
{
    public static function assertEquals($expected, $actual, $memo='')
    {
        return (new AssertLogic())->assertEquals($expected, $actual, $memo);
    }
}

Then extend TestCase against AssertLogic. Then literally everyone can be happy, at probably minimal overhead. (if the news are too much, then private static $asserter.

magento-devops-reposync-svc pushed a commit that referenced this issue Aug 24, 2021
…-coding-standard-224

[Imported] AC-958: create unit test for Magento2\Annotation checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted New rule is accepted new rule New feature implementation Progress: good first issue Issues is easy to get started with technical guidelines The rule is based on Magento Technical Guidelines
Projects
None yet
Development

No branches or pull requests

5 participants