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

feat(encoding): introduce Base64\Variant enum to support encoding/decoding different variants #408

Merged
merged 9 commits into from
Jun 16, 2023

Conversation

Gashmob
Copy link
Contributor

@Gashmob Gashmob commented Jun 5, 2023

Closes issue #407

This pr introduces functions url_encode and url_decode in Psl\Encoding\Base64 namespace. Following rfc4648 it uses base64url alphabet to make base64 encoding url safe.

issue azjezz#407

This commit introduces functions url_encode and url_decode in
Psl\Encoding\Base64 namespace. Following rfc4648 it uses base64url
alphabet to make base64 encoding url safe.
src/Psl/Encoding/Base64/url_encode.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/url_decode.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/url_encode.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/url_decode.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/url_decode.php Outdated Show resolved Hide resolved
@azjezz azjezz changed the title Base64 url feat(encoding): introduce Base64\url_encode and Base64\url_decode functions Jun 8, 2023
@azjezz azjezz added Priority: Medium This issue may be useful, and needs some attention. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably ask for additions or changes. labels Jun 8, 2023
Co-authored-by: Saif Eddin Gmati <29315886+azjezz@users.noreply.github.com>
@coveralls
Copy link

coveralls commented Jun 8, 2023

Pull Request Test Coverage Report for Build 5281085370

  • 138 of 144 (95.83%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 99.042%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Psl/Encoding/Base64/Internal/Base64.php 84 90 93.33%
Totals Coverage Status
Change from base Build 5012888631: -0.1%
Covered Lines: 4137
Relevant Lines: 4177

💛 - Coveralls

In Base64 namespace:

For both encode and url_encode, add bool $padding arg to tell if we have
or not padding at the end. By default encode is set to true and
url_encode to false.

For both decode and url_decode, add bool $explicitPadding arg to tell if
we check or not padding at the end. By default decode is set to true and
url_decode to false.
In goal to do dot slash [ordered] encoding, refactor base64 encoding
with inspiration from ParagonIE
(https://github.com/paragonie/constant_time_encoding).
src/Psl/Encoding/Base64/Internal/Base64DotSlashOrdered.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/Internal/Base64DotSlash.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/Internal/Base64UrlSafe.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/decode.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/decode.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/encode.php Show resolved Hide resolved
src/Psl/Encoding/Base64/Internal/Base64.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/Internal/Base64DotSlash.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/Internal/Base64DotSlashOrdered.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/Internal/Base64UrlSafe.php Outdated Show resolved Hide resolved
@Gashmob Gashmob requested a review from azjezz June 15, 2023 16:07
@azjezz
Copy link
Owner

azjezz commented Jun 15, 2023

@veewee do you think this should start a new major branch ( 3.x ), or should we allow this BC break to go through?

we have before release a minor with a bc break in a non major component, so that won't be new :)

@veewee
Copy link
Collaborator

veewee commented Jun 16, 2023

@azjezz the way I see this, is that the public signature of the method did not change in a breaking way. The only thing that changed is extra optional arguments and the internal implementation.
I don't think we need a major got that.

Do we need to benchmark the implementation vs PHPs implementation to make sure it's similarly performant and produces the same results?

@azjezz
Copy link
Owner

azjezz commented Jun 16, 2023

I don't think we need a major got that.

It did though, we added new Variant $variant argument before the old bool $pad argument, while both are optional, something like Base64\encode($data, true) will fail now.

@azjezz
Copy link
Owner

azjezz commented Jun 16, 2023

Do we need to benchmark the implementation vs PHPs implementation to make sure it's similarly performant and produces the same results?

we can, but keep in mind that this implementation while slower ( not sure by how much ), provides constant time encoding/decoding, which the PHP implementation does not provide.

@veewee
Copy link
Collaborator

veewee commented Jun 16, 2023

I don't think we need a major got that.

It did though, we added new Variant $variant argument before the old bool $pad argument, while both are optional, something like Base64\encode($data, true) will fail now.

Previously we did not provide any padding options though. It was just the encoding/encoded arg

@azjezz
Copy link
Owner

azjezz commented Jun 16, 2023

never mind me then :) my brain is not working right.

src/Psl/Encoding/Base64/Internal/Base64.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/Internal/Base64.php Outdated Show resolved Hide resolved
src/Psl/Encoding/Base64/Variant.php Outdated Show resolved Hide resolved
@azjezz
Copy link
Owner

azjezz commented Jun 16, 2023

@Gashmob aside from the comments above, this looks great to me 💪 we can ship it in the next minor soon.

@azjezz azjezz changed the title feat(encoding): introduce Base64\url_encode and Base64\url_decode functions feat(encoding): introduce Base64\Variant enum to support encoding/decoding different variants Jun 16, 2023
@Gashmob
Copy link
Contributor Author

Gashmob commented Jun 16, 2023

Ok, I've applied your suggestions and fix errors from checks. Normally all it's done, clean, good 😄

@azjezz azjezz added Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness and removed Status: In Progress This issue is being worked on, and has someone assigned. labels Jun 16, 2023
@azjezz azjezz merged commit a5466a3 into azjezz:next Jun 16, 2023
@azjezz
Copy link
Owner

azjezz commented Jun 16, 2023

awesome work @Gashmob !

thank you for contributing 🎉

@Gashmob Gashmob deleted the base64_url branch June 17, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants