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

Allow doctrine/lexer 2 #460

Merged
merged 1 commit into from Dec 11, 2022
Merged

Allow doctrine/lexer 2 #460

merged 1 commit into from Dec 11, 2022

Conversation

greg0ire
Copy link
Member

It is still possible to avoid a BC-break while doing so.

@greg0ire
Copy link
Member Author

SA is broken, I will fix it once doctrine/lexer 2 is removed, but I just wanted to show here that things are OK according to PHPUnit.

@@ -126,4 +126,12 @@ protected function getType(&$value)

return $type;
}

/** @return array{value: int|string, type:string|int|null, position:int} */
public function peek(): array
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to also override glimpse since it returns whatever peek() returns.

It is still possible to avoid a BC-break while doing so.
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Sacrificing the Liskov substitution principle on the altar of backwards compatibility. 🤞🏻

@greg0ire greg0ire added this to the 1.14.0 milestone Dec 11, 2022
@greg0ire greg0ire merged commit 23bf490 into doctrine:1.14.x Dec 11, 2022
@greg0ire greg0ire deleted the allow-lexer-2 branch December 11, 2022 18:07
{
$token = parent::peek();

return (array) $token;
Copy link

Choose a reason for hiding this comment

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

Should the array type always be forced here? In v1.13.x that was not the case because the method was inherited from the parent class. The docblock does not state that an empty array could be returned, but that is exactly what will be returned when the parent returns null right now.

Copy link
Member

Choose a reason for hiding this comment

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

No, we should not cast null to an array. Up for a PR?

Copy link

Choose a reason for hiding this comment

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

Fixed in #463 🎉

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.

None yet

3 participants