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

convert issuer to string prevents php 8.1 errors #83

Merged
merged 2 commits into from
Dec 19, 2021
Merged

convert issuer to string prevents php 8.1 errors #83

merged 2 commits into from
Dec 19, 2021

Conversation

brainfoolong
Copy link
Contributor

By forcing $this->issuer to be string, even if null is set, it prevents throwing errors in PHP 8.1 is rawurlencode is not allowed to have null as parameter.

It would be better to force string to be already in __construct, but this may create a breaking change for existing users.

By forcing $this->issuer to be string, even if null is set, it prevents throwing errors in PHP 8.1 is `rawurlencode` is not allowed to have null as parameter.

It would be better to force string to be already in `__construct`, but this may create a breaking change for existing users.
@willpower232
Copy link
Collaborator

Thanks for this, can you add 8.1 to the list of checked versions here and see if there are any other problems reported by tests?

php-version: ['5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0']

@brainfoolong
Copy link
Contributor Author

Hi, i would like to but unfortunately i don't have time to setup a test environment anytime soon.

@willpower232
Copy link
Collaborator

I've just approved running github actions here so if you add that then github actions will do all the testing publicly

@willpower232
Copy link
Collaborator

Github can't reproduce the original issue in master, are you able to share the code you used to trigger the problem?

@brainfoolong
Copy link
Contributor Author

For sure

$tfa = new TwoFactorAuth();
$tfa->getQRText("test", "secret");

@MasterOdin
Copy link
Contributor

See https://github.com/MasterOdin/TwoFactorAuth/commit/66687ba1729125792085543fe7a62801bcf55cad (and failing PHP 8.1 action) which adds a new test case that demonstrates the bug. This also required setting php.ini value as part of setup-php action as for PHP 8.0+, it uses a production php.ini file that sets E_ALL & ~E_DEPRECATED & ~E_STRICT and so would miss the failing test by default (ref: shivammathur/setup-php#450).

@willpower232
Copy link
Collaborator

thanks for confirming @MasterOdin, are you able to PR that extra check into this repo as well?

@MasterOdin
Copy link
Contributor

The testcase should probably be included into this PR by @brainfoolong as it fails otherwise. I can make a PR though with the change to setup-php and phpunit.xml configuration.

@willpower232 willpower232 merged commit e76f31e into RobThree:master Dec 19, 2021
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