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

Migrate to doctrine/lexer v3.0 #339

Closed
wants to merge 10 commits into from
Closed

Migrate to doctrine/lexer v3.0 #339

wants to merge 10 commits into from

Conversation

Ph0tonic
Copy link
Contributor

@Ph0tonic Ph0tonic commented Dec 14, 2022

Doctrine/L just released a brand new version 3.0 and here is a PR to use this newer version.

Let me know @egulias if I need to change anything, I am not familiar with psalm annotations.

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, thanks for taking the time. Very nice PR to update the dependency, at last it brings a Token type.
I have made a bunch of comments but please take a close look to Scrutinizer comments on the files. There are methods in the new version of the Lexer library that have types different to the one in the code.
Also check the Checks tab and see why tests are failing.

Looking forward to the changes. And thanks again for the PR!

src/EmailLexer.php Outdated Show resolved Hide resolved
src/EmailLexer.php Outdated Show resolved Hide resolved
src/EmailLexer.php Outdated Show resolved Hide resolved
src/EmailLexer.php Outdated Show resolved Hide resolved
src/EmailLexer.php Outdated Show resolved Hide resolved
src/EmailLexer.php Outdated Show resolved Hide resolved
src/MessageIDParser.php Outdated Show resolved Hide resolved
@Ph0tonic Ph0tonic changed the title Migrate to doctrine/lexer v2.0 Migrate to doctrine/lexer v2.1 Dec 14, 2022
@Ph0tonic
Copy link
Contributor Author

Ph0tonic commented Dec 14, 2022

Hey @egulias ,

Thank for your feedback, I applied them and fixed the tests. There was a type check issue due to the value null of the lex S_EMPTY. I changed it to -1 and made the relevant changes to make it work.

I also finally managed to fix all Psalm issues so I'll let you review the changes 👍

@Pasqualle
Copy link

doctrine/lexer v3.0.0 is also available now:
https://packagist.org/packages/doctrine/lexer

@Ph0tonic
Copy link
Contributor Author

Ph0tonic commented Dec 18, 2022

doctrine/lexer v3.0.0 is also available now: https://packagist.org/packages/doctrine/lexer

Thank for the info @Pasqualle

@egulias The main difference with this new v3.0.0 version is the dropped support for PHP <8.1. From what I have read in your readme, we should create a new v4.x release branch.
I updated PHP versions in Travis, with the newer supported version but I wasn't sure about the support of ppc64le architecture. Let me know what you think about that. 👍

@Ph0tonic Ph0tonic changed the title Migrate to doctrine/lexer v2.1 Migrate to doctrine/lexer v3.0 Dec 18, 2022
@derrabus
Copy link
Contributor

Note that Lexer 3 is Lexer 2 minus the deprecation layer, so requiring Lexer 3 without allowing v2 as well is not really necessary. Lexer 3 is not supported by Doctrine ORM 2 and it likely won't ever be. This basically means that bumping to "doctrine/lexer": "^3" instead of "doctrine/lexer": "^2 || ^3" would forbid the use of this package in a project that also uses Doctrine ORM.

@derrabus derrabus mentioned this pull request Dec 20, 2022
@derrabus
Copy link
Contributor

I've submitted #340 to unblock Lexer 2 (and doctrine/annotations 2) in Symfony's CI.

@egulias
Copy link
Owner

egulias commented Dec 26, 2022

Thanks @derrabus, very useful.
Will merge #340 and then upgrade this library to v4 with this code.
Will work on this during the week.

@Ph0tonic I might need you to do a PR to another branch for this. Will let you know.

Thanks guys!

@Ph0tonic
Copy link
Contributor Author

Thanks @derrabus, very useful. Will merge #340 and then upgrade this library to v4 with this code. Will work on this during the week.

@Ph0tonic I might need you to do a PR to another branch for this. Will let you know.

Thanks guys!

Great thank you, let me know if you need me to do anything.

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.

We are almost there! A couple of more changes please.
I'll create the 4.x branch and will request you to do the PR to that branch, once we have sorted out all the changes on this PR (so keep the conversation in one place).

Thanks!

README.md Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@Ph0tonic
Copy link
Contributor Author

Great thank you for your feedback. Here are the requested changes. 👍

composer.json Outdated
"doctrine/lexer": "^1.2",
"symfony/polyfill-intl-idn": "^1.15"
"php": ">=8.1",
"doctrine/lexer": "^3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I would strongly recommend to loosen this constraint to…

Suggested change
"doctrine/lexer": "^3.0",
"doctrine/lexer": "^2.0 || ^3.0",

This should be possible without much extra effort and avoid dependency conflicts downstream for the time being.

Copy link
Owner

Choose a reason for hiding this comment

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

I have checked Doctrine's ORM and v3 is requiring v2 of the lexer, so @Ph0tonic can you please update to

Suggested change
"doctrine/lexer": "^3.0",
"doctrine/lexer": "^2 || ^3",

Thanks @derrabus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@egulias
Copy link
Owner

egulias commented Dec 29, 2022

@Ph0tonic please close this PR and create a new one against v4 branch: https://github.com/egulias/EmailValidator/tree/4.x .
I'll take care of the conflicts.
Thnkas!

@egulias
Copy link
Owner

egulias commented Jan 2, 2023

Closing in favor of #344

@derrabus
Copy link
Contributor

derrabus commented Jan 2, 2023

You mean #344, I guess. 🙃

@egulias
Copy link
Owner

egulias commented Jan 2, 2023

Updated, thanks!

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.

4 participants