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

add support for endroid/qr-code version 6 #140

Merged

Conversation

cliffordvickrey
Copy link
Contributor

Feature added

Updated EndroidQrCodeProvider to work with version 6 of optional endroid/qr-code repository. (Breaking change in dependency: setter methods removed in favor of more modern practice of passing properties to QrCode constructor)

Testing

Confirmed that EndroidQRCodeTest passes with either version 5 or 6 of endroid/qr-code installed

Copy link
Collaborator

@willpower232 willpower232 left a comment

Choose a reason for hiding this comment

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

Thanks for your help, can you also add 6 to .github/workflows/test-endroid.yml to ensure it gets tested?

@cliffordvickrey
Copy link
Contributor Author

Thank you for looking at this so quickly! ^6 has been added to the list of tested package versions

@NicolasCARPi
Copy link
Collaborator

I suggest dropping versions 3 and 4 from our code/test. No need to try and be compatible with php 5.6 era libraries. Not necessarily in this PR, but adding support for a new version should come with dropping support for the oldest one, too. Or you end up with too many codepaths, maintenance cost, attack surface, etc...

@willpower232
Copy link
Collaborator

Its no skin off my back, they do still install on PHP 8 though so we'd have to add a conflicts in composer.json to reject those versions right?

Probably easier to pull off next year with PHP 9 😅

@willpower232 willpower232 merged commit 92487ac into RobThree:master Oct 24, 2024
14 checks passed
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.

3 participants