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

Create common utility to handle exceptions lists #982

Merged
merged 6 commits into from
May 1, 2024

Conversation

kylekatarnls
Copy link
Member

Type: refactoring
Breaking change: yes (protected scope)

BC: getExceptionsList() can be called from custom rules that would extends StaticAccess, TooManyMethds, LongClassName, ShortClassName, LongVariable, ShortVariable, ShortMethodName or UnusedLocalVariable previously returned array and will now return a object implementing IteratorAggregate, ArrayAccess so if user iterates over it or call isset($exceptions[$key]) the resulting behavior will be the same, but if they expect exactly an array (strong typing or using some array_ method on it), they will need to replace $this->getExceptionList() with iterator_to_array($this->getExceptionList()) also if they overridden getExceptionList() in a sub-class, they will need to wrap the returned value in a class like:

class ArrayWrapper
{
    public function __construct(private array $list)
    {
    }

    public function contains(string $key): bool
    {
        return in_array($key, $this->list, true);
    }
}

Risk of BC for real users is low as users are encouraged to extends AbstractRule to create their own, there is little interest to extend an actual rule (BTW, they should likely be final in next major version, and have their methods private)

Motivation: we have all the listed rules above having a getExceptionList() method, all with very close code:

  • 1 variant uses array_flip on the list then call isset instead of in_array, this variant add a fixed cost per activated rule but reduces to cost per iteration (method/class/function/variable). As per my benchmark it will benefit to each rule checking more than 13 times the exceptions list, so it would benefit to most of the projects/usages when PHPMD is given big code base, and it would make slower only analyses of very small project or partial analyze (such as a single-file check). So, in my opinion it makes sense to use the isset() variant for all rules.
  • due to slightly different method of trim/filter, ,, would in some rules generate a blank exception, in some other not, in some case whitespace around comas will be ignored but not in all rules. It sounds those differences are not wanted and just edge-case bugs. Harmonizing them would ensure a given exceptions could be used for any rule in the config and be handled the same way.
  • some rules also trim \ with whitespace, this makes sense only for class name, so I make it as a parameter and only exceptions list meant to be checked against class name have it, other get the default value "" so only whitespace is trimmed.

Apart the $trim parameter then now rules supporting exceptions config will all have the same method implementation with minimum code:

    protected $exceptions;

    protected function getExceptionsList()
    {
        if ($this->exceptions === null) {
            $this->exceptions = new ExceptionsList($this);
        }

        return $this->exceptions;
    }

After dropping PHP 5.3 this can became a trait and minimize the duplicate code accros rules.

#924 will also require to add this config.

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2022

Codecov Report

Attention: Patch coverage is 80.95238% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 89.09%. Comparing base (412a07d) to head (43266a5).
Report is 61 commits behind head on 3.x.

❗ Current head 43266a5 differs from pull request most recent head 406f7b9. Consider uploading reports for the commit 406f7b9 to get more accurate results

Files Patch % Lines
src/main/php/PHPMD/Utility/ExceptionsList.php 55.55% 12 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                3.x     #982      +/-   ##
============================================
- Coverage     92.66%   89.09%   -3.57%     
+ Complexity     1238     1197      -41     
============================================
  Files           111      110       -1     
  Lines          3447     3073     -374     
============================================
- Hits           3194     2738     -456     
- Misses          253      335      +82     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AJenbo
Copy link
Member

AJenbo commented Sep 11, 2022

I like this change, but I think it would be best to have it as part of the 3.0 branch because of the difference of behavior and signatures.

@kylekatarnls kylekatarnls added this to the 3.0.0 milestone Sep 11, 2022
@kylekatarnls
Copy link
Member Author

OK, then I'll handle #924 using array version. 👌

@AJenbo AJenbo changed the base branch from master to 3.x May 1, 2024 22:00
@AJenbo AJenbo merged commit 59c53fa into phpmd:3.x May 1, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants