rectorphp / rector-phpunit

Rector upgrade rules for PHPUnit
http://getrector.com
MIT License
61 stars 46 forks source link

NamedArgumentForDataProviderRector only changing the first data provider #345

Closed gnutix closed 6 days ago

gnutix commented 1 week ago

Hey there,

I've ran NamedArgumentForDataProviderRector on my project and noticed it only refactors the first #[DataProvider] it encounters. On my project, it missed about a 1/3 of the deprecations.

Changing getDataProviderMethodName to return an array of method names :

    private function getDataProviderMethodNames(ClassMethod $classMethod) : array
    {
        $methodNames = [];
        $attributeClassName = DataProvider::class;
        foreach ($classMethod->attrGroups as $attributeGroup) {
            foreach ($attributeGroup->attrs as $attribute) {
                if (!$this->isName($attribute->name, $attributeClassName)) {
                    continue;
                }
                foreach ($attribute->args as $arg) {
                    if ($arg->value instanceof String_) {
                        $methodNames[] = $arg->value->value;
                    }
                }
            }
        }
        return $methodNames;
    }

and calling it like this :

            $dataProviderMethodNames = $this->getDataProviderMethodNames($classMethod);
            if ($dataProviderMethodNames === []) {
                continue;
            }
            foreach ($dataProviderMethodNames as $dataProviderMethodName) {
                // ... rest of the code untouched
            }

fixes the issue. /cc @marcelthole

gnutix

TomasVotruba commented 1 week ago

Great :+1: Could you send the PR with fix? :)

gnutix commented 1 week ago

Sorry, too much on my plate these days.

TomasVotruba commented 1 week ago

No worries :+1:

Closing as duplicate of similar issue to keep focus: https://github.com/rectorphp/rector-phpunit/issues/346#issuecomment-2198370779

gnutix commented 1 week ago

Without this fix, the rule is only partially doing its job, and the "optional feature" triggers deprecations that are outputted by PHUnit. So please, don't close this issue, it's a separate problem from the other you've linked.

TomasVotruba commented 1 week ago

I missunderstood then. We'll need a failing demo link first.

Ref: https://github.com/rectorphp/rector-phpunit/pull/330

/cc @marcelthole

marcelthole commented 6 days ago

I added a failing testcase and fixed that case. We didn't had the case with multiple dataproviders on a single test method. But it should be fixed now :)