Skip to content

Implemented new ChartGoogleQrCodeProvider #79

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

Merged
merged 5 commits into from
Oct 20, 2021

Conversation

fman42
Copy link
Contributor

@fman42 fman42 commented Oct 3, 2021

Hello!

Implemented new provider that using Google API for generating QR

I think it will be helpful somebody like me who need use Google API within this package

@RobThree
Copy link
Owner

RobThree commented Oct 3, 2021

Wouldn't GoogleChartsQrCodeProvider be a more logical name? Also, shouldn't VerifySSL default to true? Maybe even make it an optional constructor argument?

Other than that (haven't tested it, but assuming it works): looks good to me!

@fman42
Copy link
Contributor Author

fman42 commented Oct 4, 2021

Yep, name GoogleChartsQrCodeProvider is more logical. I made verifySSL property like as optional argument in constructor. It will give more independence

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.

Looks fine to me although it should probably reference the Image Charts provider where verifySSL is the first parameter of the function and is also double checked to confirm it is a boolean.

public function __construct($verifyssl = false, $errorcorrectionlevel = 'L', $margin = 1)
{
if (!is_bool($verifyssl)) {
throw new QRException('VerifySSL must be bool');
}
$this->verifyssl = $verifyssl;

@fman42
Copy link
Contributor Author

fman42 commented Oct 9, 2021

Looks fine to me although it should probably reference the Image Charts provider where verifySSL is the first parameter of the function and is also double checked to confirm it is a boolean.

public function __construct($verifyssl = false, $errorcorrectionlevel = 'L', $margin = 1)
{
if (!is_bool($verifyssl)) {
throw new QRException('VerifySSL must be bool');
}
$this->verifyssl = $verifyssl;

I think it will be redundant (check the boolean type). The user can, for example, pass the "string" in the "margin" parameter.

@willpower232
Copy link
Collaborator

In this case, the value is passed directly into curl which means if the library doesn't double check about the boolean and a value which isn't literally a boolean is passed into curl then I'm not sure the behaviour would be what the developer desires.

CURLOPT_SSL_VERIFYPEER => $this->verifyssl,

It has been mentioned before that consistency is important so these libraries should be very similar if not identical.

I notice that you've added an encoding parameter that doesn't seem to get used so technically, could you extend the ImageChartsQRCodeProvider instead of the base one and then you only need to override one function?

@fman42
Copy link
Contributor Author

fman42 commented Oct 17, 2021

Yeah, I saw code structure attentively and have pushed a new commit

Thank you for your comment about the encoding option. Inserted that option in the query to Google API.
I have visited https://documentation.image-charts.com/qr-codes/ and noticed: there is has 'choe' too. I can add that in ImageChartsQRCodeProvider by another Pull Request and then refactoring with extending of this class

An extend the ImageChartsQRCodeProvider forces to add a parameter there and not use it

@RobThree
Copy link
Owner

RobThree commented Oct 18, 2021

I was experiencing a déjà vu but now I remember why 😂 I'm actually not quite sure why it got removed in the first place...

Anyway, I'm sorry this took a while; I've been quite busy. I hope to get around to merging this PR this week.

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.

🙏 LGTM

@RobThree RobThree merged commit b79d438 into RobThree:master Oct 20, 2021
@RobThree
Copy link
Owner

Merged and published 1.8.1. Thanks @fman42 and @willpower232 🙏

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