Skip to content

Improve ArrayKeyExists specifyingExtension#1941

Merged
ondrejmirtes merged 5 commits intophpstan:1.9.xfrom
VincentLanglet:arrayKeyExist
Jan 17, 2023
Merged

Improve ArrayKeyExists specifyingExtension#1941
ondrejmirtes merged 5 commits intophpstan:1.9.xfrom
VincentLanglet:arrayKeyExist

Conversation

@VincentLanglet
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Contributor

@herndlm herndlm Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jfyi we have isString() already too which I personally prefer. But it should come down to the same thing I guess. UPDATE: this was meant for the line above, sorry

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed

@voku
Copy link
Copy Markdown
Contributor

voku commented Nov 2, 2022

Maybe you can add some extra tests from this old pull request? 🤔 : #636

@VincentLanglet
Copy link
Copy Markdown
Contributor Author

VincentLanglet commented Nov 2, 2022

Maybe you can add some extra tests from this old pull request? 🤔 : #636

I take a look and there is only 6 check about array_key_exists and I feel like they already covered by the test:

if (array_key_exists($key, $b)) {
     assertType("'3'|'foo'", $key);
}

But I can add more tests if needed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VincentLanglet
Copy link
Copy Markdown
Contributor Author

Just rebased the branch if you have time to review @ondrejmirtes

To me the test failure are correct, cf #1941 (comment)

@VincentLanglet VincentLanglet force-pushed the arrayKeyExist branch 2 times, most recently from 08daa9d to ed2c1f7 Compare December 17, 2022 14:43
@VincentLanglet VincentLanglet requested review from ondrejmirtes and removed request for herndlm December 17, 2022 14:56
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like some more tests about tricky situations. Let's say we have an array shape like:

$a = [3 => 'foo', 4 => 'bar']

When we have string $key and we ask about array_key_exists($key, $a), the $key should become '3'|'4'.

It should work generally, so int|string should become 3|4|'3'|'4'.

@VincentLanglet
Copy link
Copy Markdown
Contributor Author

I'd like some more tests about tricky situations. Let's say we have an array shape like:

$a = [3 => 'foo', 4 => 'bar']

When we have string $key and we ask about array_key_exists($key, $a), the $key should become '3'|'4'.

It should work generally, so int|string should become 3|4|'3'|'4'.

Test added with a little fix to get the expected behavior

@VincentLanglet
Copy link
Copy Markdown
Contributor Author

For the build failure:
https://github.com/phpstan/phpstan-src/actions/runs/3869539129/jobs/6595709562

/**
     * Apply filters to address query builder.
     *
     * @param array $filters
     * @param QueryBuilder $qb
     */
    private function applyFilters(QueryBuilder $qb, array $filters)
    {
        $allowedFiltersMap = [
            'id_customer' => 'a.id_customer',
        ];

        foreach ($filters as $filterName => $value) {
            if (!array_key_exists($filterName, $allowedFiltersMap) || empty($value)) {
                continue;
            }

            if ('id_customer' === $filterName) {
                $qb->andWhere('a.`id_customer` = :' . $filterName);
                $qb->setParameter($filterName, $value);

                continue;
            }

            $qb->andWhere($allowedFiltersMap[$filterName] . ' LIKE :' . $filterName)
                ->setParameter($filterName, '%' . $value . '%');
        }
    }

The end of the foreach is indeed never reach, because of the array_key_exists check, the only $filterName value allowed is 'id_customer' which is handled by the following if.

https://github.com/phpstan/phpstan-src/actions/runs/3869539129/jobs/6595709711

if (array_key_exists($join['joinAlias'], $knownAliases)) {
    throw QueryException::nonUniqueAlias($join['joinAlias'], array_keys($knownAliases));
}

The fact is in the doctrine code, we are inside a foreach, and we're modifying $knowAliases by setting new value with keys that are mixed, so the initial array<string, mixed> array become array<int|string, mixed>. Therefor the new error reported by PHPStan is correct too.

@VincentLanglet
Copy link
Copy Markdown
Contributor Author

@ondrejmirtes Do I need to target 1.10.x now ?

@ondrejmirtes
Copy link
Copy Markdown
Member

@VincentLanglet No #2043 (comment)

@ondrejmirtes ondrejmirtes merged commit 980551b into phpstan:1.9.x Jan 17, 2023
@ondrejmirtes
Copy link
Copy Markdown
Member

Thank you.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Array (array<string, string|null>) does not accept key string.

4 participants