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

BUGFIX: Union type handling #3439

Draft
wants to merge 3 commits into
base: 8.3
Choose a base branch
from

Conversation

mficzel
Copy link
Member

@mficzel mficzel commented Jan 23, 2025

Demonstrate broken union types from annotations via test.

  1. commit - remove rule that tests union types only with php 8 ... all green
  2. commit - add a method with union type parameter hints and test thererfore ... still green
  3. commit - add a class similar to 2. but with @param and @return annotations instead of type-hints ... tests verify that the same types are detected as with type hints ... and fail

Please note tha added case parameterB1 is equel to parameterB but demonstrates the different handlung of spaces in annotations that lead to detecting mixed type.

see: #3440

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@github-actions github-actions bot added the Bug label Jan 23, 2025
@mficzel mficzel changed the base branch from 9.0 to 8.3 January 23, 2025 09:37
@github-actions github-actions bot added 9.0 8.3 and removed 9.0 labels Jan 23, 2025
@mficzel mficzel force-pushed the bugfix/unionTypeHandling branch 5 times, most recently from 493078a to 8df5aa8 Compare January 23, 2025 12:52
@mficzel
Copy link
Member Author

mficzel commented Jan 23, 2025

I think ...

  1. the @param annotation should be read with a regex and not explode( ' ', ...)
  2. the expanType in the reflectionService has to learn about UnionTypes and probably intersectioTypes

self::assertEquals(
[
'returnTypeA' => 'string|false',
'returnTypeB' => '\Neos\Flow\Tests\Functional\Reflection\Fixtures\DummyClassWithUnionTypeHints|false',
Copy link
Member

Choose a reason for hiding this comment

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

In PHP FQCN in strings never have a leading backslash – should we handle that the same way? At least consistency would be great, seeing the next test expects those without a leading backslash…

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests just took what the type hints created for granted. May aswell be buggy.

self::assertEquals(
[
'returnTypeA' => 'string|false',
'returnTypeB' => '\Neos\Flow\Tests\Functional\Reflection\Fixtures\DummyClassWithUnionTypeAnnotations|false',
Copy link
Member

Choose a reason for hiding this comment

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

See earlier comment about leading backslash…

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

Successfully merging this pull request may close these issues.

2 participants