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

Fixed hooks parameters and return types #1466

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Conversation

kukulich
Copy link
Collaborator

Just another bug found by @ondrejmirtes

@kukulich kukulich added the bug label Dec 11, 2024
@kukulich kukulich marked this pull request as draft December 11, 2024 20:40
@kukulich kukulich force-pushed the hooks branch 2 times, most recently from 723c669 to ab81459 Compare December 11, 2024 21:28
@kukulich kukulich marked this pull request as ready for review December 12, 2024 08:26
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

🚢 thanks @kukulich!

Comment on lines -42 to +48
private array $parameters;
protected array $parameters;

/** @psalm-allow-private-mutation */
private bool $returnsReference;

/** @psalm-allow-private-mutation */
private ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null $returnType;
protected ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null $returnType;
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of using protected (plus a trait), so we may really need to do some changing here in the next versions.

#[DataProvider('getPropertyHookReturnTypeProvider')]
public function testGetPropertyHookReturnType(string $propertyName, string|null $returnType): void
{
$reflector = new DefaultReflector(new SingleFileSourceLocator(__DIR__ . '/../Fixture/PropertyHooks.php', $this->astLocator));
Copy link
Member

Choose a reason for hiding this comment

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

One note on the design of these tests: having everything inside a third-party file makes them a bit harder to locate when jumping around source, compared to using a string locator inlined with the test

@Ocramius Ocramius self-assigned this Dec 12, 2024
@Ocramius Ocramius added this to the 6.46.0 milestone Dec 12, 2024
@Ocramius Ocramius merged commit 9cfae68 into Roave:6.46.x Dec 12, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants