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

Changed DocBlock param type invalid or not consistent #62

Closed
sbuerk opened this issue Feb 26, 2022 · 6 comments
Closed

Changed DocBlock param type invalid or not consistent #62

sbuerk opened this issue Feb 26, 2022 · 6 comments

Comments

@sbuerk
Copy link

sbuerk commented Feb 26, 2022

After upgrading dependencies which includes this package from 1.2.1 to 1.2.2 phpstan now complaints because of invalid types for a method call.

I have commented it directly here: 863bff4#r67532655

In the TYPO3 implementation we have a call which uses the Lexer public class constants, which are integer typed constants to check thinks, like:

$isNextToken = $lexer->isNextTokenAny([Lexer::T_INDEX, Lexer::T_KEY]);

phpstan now complains because this do not match with the changed docblock type for the param from array to string[].

Not sure, either the docblock should be changed to int[]|string[] or mixed[] to mitigate this. Not sure about the whole implemention. If token keywords should be string, the class constants should be changed too.

Example error message from phpstan:

  1177   Parameter #1 $tokens of method                                 
         Doctrine\Common\Lexer\AbstractLexer::isNextTokenAny() expects  
         array<string>, array<int, int> given.
@greg0ire
Copy link
Member

@sbuerk why don't you send a PR? In #48, I did what I think was right but I think I know the library less than you do (I don't use it directly). Moreover, if you manually edit the phpdoc in your project, you will be able to validate that things go back to normal (no static analysis errors), or that more changes are needed.

@sbuerk
Copy link
Author

sbuerk commented Feb 27, 2022

@greg0ire Wanted to report this issue first. For the case some may/can confirm this issue or have a pr ready faster then I - had to deal with a lot of forks yesterday to takle upraising bugs because of "3rd party" changes in several projects - that was the reason why not directly adding a "pr" - which would have been a "quick shot".

Try to test this out deeper today and eventually adding a PR for this.

@greg0ire
Copy link
Member

Great! Looking forward to it!

@greg0ire
Copy link
Member

greg0ire commented Feb 27, 2022

Regarding your remark, I'd go with list<int|string>, or even list<int>|list<string>, as I don't think another type would be very reasonable to use.

@sbuerk
Copy link
Author

sbuerk commented Feb 27, 2022

I'm currently on it. Already added a test ConcreteLexer with integer tokens, adopted from the string based token tester. However, psalm do not detect the mismatch in these changes.

I think these tests are still not useless, as it tests the integer based tokens. Our implementation was based on the documentation, and thus assuming integer based tokens are a valid implementation.

See: https://www.doctrine-project.org/projects/doctrine-lexer/en/1.2/simple-parser-example.html#simple-parser-example

We use phpstan in our project which reports this missmatch however. Will test your suggestions if phpstan will work with that and use that docblocks then instead of already proved working int[]|string[]. And union type for isNextTokenAny() is only a concequence and adoption of the isNextToken() and other methods using int|string for token types.

sbuerk added a commit to sbuerk/lexer that referenced this issue Feb 27, 2022
This patch changes the docblock type hint of `AbstractLexer::isNextTokenAny()`
to `list<int|string>` to adopt the int and string based support of token types.
Thus making phpstan in projects happy which uses this packages for there own
lexer implementations.

However this only aligns to other token methods which already supports int and
string as token types, thus fixing a wrong set type hint through doctrine#48.

Fixes: doctrine#62
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this issue Feb 27, 2022
doctrine/lexer has released a new minor version with changed
method docblocks, which now emits phpstan errors because of
incompatible types.

This patch raises the minor version for development and core
usage and adding phpstan ignore pattern to the baseline file
until doctrine/lexer has fixed the incompatible state. This
is a dedicated preparation to raise other dev dependencies.

Issue has been reported to the corresponding github repository:
doctrine/lexer#62

used commands:

> composer req doctrine/lexer:"^1.2.2"
> composer req doctrine/lexer:"^1.2.2" \
  --no-update -d typo3/sysext/core
> Build/Scripts/runTests.sh \
  -s phpstanGenerateBaseline

Resolves: #97055
Releases: main, 11.5
Change-Id: Ib5c04202bdc6a4b5787a191e4bf1e175982fb217
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73729
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Nikita Hovratov <nikita.h@live.de>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Nikita Hovratov <nikita.h@live.de>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this issue Feb 27, 2022
doctrine/lexer has released a new minor version with changed
method docblocks, which now emits phpstan errors because of
incompatible types.

This patch raises the minor version for development and core
usage and adding phpstan ignore pattern to the baseline file
until doctrine/lexer has fixed the incompatible state. This
is a dedicated preparation to raise other dev dependencies.

Issue has been reported to the corresponding github repository:
doctrine/lexer#62

used commands:

> composer req doctrine/lexer:"^1.2.2"
> composer req doctrine/lexer:"^1.2.2" \
  --no-update -d typo3/sysext/core
> Build/Scripts/runTests.sh \
  -s phpstanGenerateBaseline

Resolves: #97055
Releases: main, 11.5
Change-Id: Ib5c04202bdc6a4b5787a191e4bf1e175982fb217
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73729
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Nikita Hovratov <nikita.h@live.de>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Nikita Hovratov <nikita.h@live.de>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this issue Feb 27, 2022
doctrine/lexer has released a new minor version with changed
method docblocks, which now emits phpstan errors because of
incompatible types.

This patch raises the minor version for development and core
usage and adding phpstan ignore pattern to the baseline file
until doctrine/lexer has fixed the incompatible state. This
is a dedicated preparation to raise other dev dependencies.

Issue has been reported to the corresponding github repository:
doctrine/lexer#62

used commands:

> composer req doctrine/lexer:"^1.2.2"
> composer req doctrine/lexer:"^1.2.2" \
  --no-update -d typo3/sysext/core
> Build/Scripts/runTests.sh \
  -s phpstanGenerateBaseline

Resolves: #97055
Releases: main, 11.5
Change-Id: Ib5c04202bdc6a4b5787a191e4bf1e175982fb217
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73736
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this issue Feb 27, 2022
doctrine/lexer has released a new minor version with changed
method docblocks, which now emits phpstan errors because of
incompatible types.

This patch raises the minor version for development and core
usage and adding phpstan ignore pattern to the baseline file
until doctrine/lexer has fixed the incompatible state. This
is a dedicated preparation to raise other dev dependencies.

Issue has been reported to the corresponding github repository:
doctrine/lexer#62

used commands:

> composer req doctrine/lexer:"^1.2.2"
> composer req doctrine/lexer:"^1.2.2" \
  --no-update -d typo3/sysext/core
> Build/Scripts/runTests.sh \
  -s phpstanGenerateBaseline

Resolves: #97055
Releases: main, 11.5
Change-Id: Ib5c04202bdc6a4b5787a191e4bf1e175982fb217
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73736
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
sbuerk added a commit to sbuerk/lexer that referenced this issue Feb 27, 2022
This patch changes the docblock type hint of `AbstractLexer::isNextTokenAny()`
to `list<int|string>` to adopt the int and string based support of token types.
Thus making phpstan in projects happy which uses this packages for there own
lexer implementations.

However this only aligns to other token methods which already supports int and
string as token types, thus fixing a wrong set type hint through doctrine#48.

Fixes: doctrine#62
sbuerk added a commit to sbuerk/lexer that referenced this issue Feb 27, 2022
This patch changes the docblock type hint of `AbstractLexer::isNextTokenAny()`
to `list<int|string>` to adopt the int and string based support of token types.
Thus making phpstan in projects happy which uses this packages for there own
lexer implementations.

However this only aligns to other token methods which already supports int and
string as token types, thus fixing a wrong set type hint through doctrine#48.

Fixes: doctrine#62
@greg0ire
Copy link
Member

reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this issue Mar 1, 2022
doctrine/lexer has released a new minor version with
fixed method docblocks, thus phpstan ignore pattern
can now be removed again.

See: doctrine/lexer#62

> composer req doctrine/lexer:"^1.2.3"
> composer req doctrine/lexer:"^1.2.3 \
  --no-update -d typo3/sysext/core
> Build/Scripts/runTests.sh \
  -s phpstanGenerateBaseline

Resolves: #97063
Related: #97055
Releases: main, 11.5
Change-Id: I5e729543c4721e7f9a17511c113139bf7908b208
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73740
Tested-by: core-ci <typo3@b13.com>
Tested-by: Benni Mack <benni@typo3.org>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>
Reviewed-by: Benni Mack <benni@typo3.org>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this issue Mar 1, 2022
doctrine/lexer has released a new minor version with
fixed method docblocks, thus phpstan ignore pattern
can now be removed again.

See: doctrine/lexer#62

> composer req doctrine/lexer:"^1.2.3"
> composer req doctrine/lexer:"^1.2.3 \
  --no-update -d typo3/sysext/core
> Build/Scripts/runTests.sh \
  -s phpstanGenerateBaseline

Resolves: #97063
Related: #97055
Releases: main, 11.5
Change-Id: I5e729543c4721e7f9a17511c113139bf7908b208
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73740
Tested-by: core-ci <typo3@b13.com>
Tested-by: Benni Mack <benni@typo3.org>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>
Reviewed-by: Benni Mack <benni@typo3.org>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this issue Mar 1, 2022
doctrine/lexer has released a new minor version with
fixed method docblocks, thus phpstan ignore pattern
can now be removed again.

See: doctrine/lexer#62

> composer req doctrine/lexer:"^1.2.3"
> composer req doctrine/lexer:"^1.2.3 \
  --no-update -d typo3/sysext/core
> Build/Scripts/runTests.sh \
  -s phpstanGenerateBaseline

Resolves: #97063
Related: #97055
Releases: main, 11.5
Change-Id: I5e729543c4721e7f9a17511c113139bf7908b208
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73754
Tested-by: core-ci <typo3@b13.com>
Tested-by: Stefan Bürk <stefan@buerk.tech>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>
Reviewed-by: Stefan Bürk <stefan@buerk.tech>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this issue Mar 1, 2022
doctrine/lexer has released a new minor version with
fixed method docblocks, thus phpstan ignore pattern
can now be removed again.

See: doctrine/lexer#62

> composer req doctrine/lexer:"^1.2.3"
> composer req doctrine/lexer:"^1.2.3 \
  --no-update -d typo3/sysext/core
> Build/Scripts/runTests.sh \
  -s phpstanGenerateBaseline

Resolves: #97063
Related: #97055
Releases: main, 11.5
Change-Id: I5e729543c4721e7f9a17511c113139bf7908b208
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73754
Tested-by: core-ci <typo3@b13.com>
Tested-by: Stefan Bürk <stefan@buerk.tech>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>
Reviewed-by: Stefan Bürk <stefan@buerk.tech>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
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 a pull request may close this issue.

2 participants