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 #340

Merged
merged 1 commit into from
Dec 29, 2022
Merged

Conversation

derrabus
Copy link
Contributor

This is a lightweight version of #339 that allows doctrine/lexer 2 to be used while maintaining backwards compatibility.

For Symfony's CI, we need this change because EmailValidator currently blocks the installation of doctrine/annotations 2 (which itself depends on Lexer 2).

This PR by no means intends to replace #339: #339 is still useful because it fully migrates this project to Lexer's new API. But since it involves BC breaks, I suggest to merge this PR into 3.x and release #339 as v4.

@derrabus derrabus force-pushed the improvement/allow-lexer-2 branch 2 times, most recently from 7b8ed65 to 2686e7d Compare December 20, 2022 12:48
*/
public $token;

/**
* The next token in the input.
*
* @var array{position: int, type: int|null|string, value: int|string}|null
* @var array|Token
Copy link
Contributor

Choose a reason for hiding this comment

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

changing the type of a public property in a non-internal class is still a BC break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an inherited BC break. This public property is declared in the Lexer library and apparently just overridden here to make the static analysis happy. As long as you pin Lexer to v1 in your project, the type won't change for you.

@derrabus
Copy link
Contributor Author

The Travis failure is unrelated and seems to be a hiccup on the build runner.

Copy link
Owner

@egulias egulias left a comment

Choose a reason for hiding this comment

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

Hey, apologies for the delay.
Can you please review all the Scrutinizr commnets? Seems there is a type miss-match.
And make v2 the only accepcted version.
Thus, you will need to change all the types to Token I believe.

psalm.xml Show resolved Hide resolved
composer.json Show resolved Hide resolved
@derrabus
Copy link
Contributor Author

Can you please review all the Scrutinizr commnets? Seems there is a type miss-match.

Scrutinizer apparently does not understand generics at all. Not sure if I can do anything about it. This part should be well covered by Psalm anyway.

@egulias
Copy link
Owner

egulias commented Dec 28, 2022

Checking Doctrine Lexer's code, it should work so we can ignore Scrutinizr, which in any case are informational anyway.

@egulias egulias merged commit b28de87 into egulias:3.x Dec 29, 2022
@derrabus derrabus deleted the improvement/allow-lexer-2 branch December 29, 2022 21:24
@derrabus
Copy link
Contributor Author

@egulias The PR was merged, but somehow one of your later merges seems to have reverted my changes. Was that on purpose?

@derrabus derrabus mentioned this pull request Dec 29, 2022
@egulias
Copy link
Owner

egulias commented Dec 29, 2022

I merged this one and #343 , I might have resolved incorrectly the conflicts.

@derrabus
Copy link
Contributor Author

I've re-submitted this PR as #345.

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.

None yet

3 participants