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

Changed QR Code #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Changed QR Code #76

wants to merge 1 commit into from

Conversation

JE4GLE
Copy link

@JE4GLE JE4GLE commented Nov 7, 2019

The QR-Code requires the title to be in front of the name seperated by ':' to be displayed in apps like Authy

The QR-Code requires the title to be in front of the name seperated by ':' to be displayed in apps like Authy
@MarkMaldaba
Copy link

This PR contains an error - it is using urlencode() inside of urlencode() meaning that the title is being escaped twice.

The problem it attempts to fix is valid and the solution seems reasonable aside from the above (though I would avoid inline short-form if statements in complex expressions and split it out into a normal if() condition to update $name before the string is built, to make the code more readable).

@MarkMaldaba
Copy link

Note that PR #80 contains an alternative solution to the same problem, though it also suffers from the double-escaping issue.

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