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

replace Endroid\QrCode with BaconQrCode #8173

Merged
merged 1 commit into from
Aug 28, 2021
Merged

Conversation

johndoh
Copy link
Contributor

@johndoh johndoh commented Aug 20, 2021

Endroid\QrCode requires min PHP 7.3 but now so does master so we can update

@alecpl
Copy link
Member

alecpl commented Aug 20, 2021

There's one problem with this update. endroid/qr-code#341

@johndoh
Copy link
Contributor Author

johndoh commented Aug 20, 2021

Ouch! I did not notice that. Perhaps we could switch to bacon/bacon-qr-code endroid requires it already any way and its much lighter weight. A very quick test suggests it should be a relatively easy switch.

$renderer = new BaconQrCode\Renderer\ImageRenderer(
    new BaconQrCode\Renderer\RendererStyle\RendererStyle(300, 1),
    new BaconQrCode\Renderer\Image\ImagickImageBackEnd()
);
$writer = new BaconQrCode\Writer($renderer);
return $writer->writeString($data);

@alecpl
Copy link
Member

alecpl commented Aug 26, 2021

I think we could do this way. The check at https://github.com/roundcube/roundcubemail/blob/master/program/actions/contacts/index.php#L345 would need to be changed to check for imagick and iconv extensions.

@johndoh johndoh changed the title update to Endroid\QrCode v4 replace Endroid\QrCode with BaconQrCode Aug 26, 2021
@johndoh
Copy link
Contributor Author

johndoh commented Aug 26, 2021

redesigned PR to replace Endroid\QrCode with BaconQrCode. BaconQrCode is already a dependency of Endroid\QrCode so results in a smaller but not totally different 3rd party dependency tree.

@alecpl alecpl merged commit 0c86320 into roundcube:master Aug 28, 2021
@johndoh johndoh deleted the qrcode branch August 28, 2021 08:15
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.

2 participants