-
Notifications
You must be signed in to change notification settings - Fork 168
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
New base64encode and sha256 filters #574
New base64encode and sha256 filters #574
Conversation
Perhaps base64 is common enough to be in the core, but if SHA256 gets added then suddenly we could end up with MD5, SHA-1, SHA-2, SHA-3 and more. I feel like there are too many hashing functions for any of them to be included in the core. Just curious, what's the use for having a hashing filter? And base64encode sounds too long, maybe just |
Sure, I'll shorten it. I thought that a simple |
6544cb5
to
913c489
Compare
913c489
to
569af95
Compare
@@ -149,6 +149,8 @@ | |||
filters.put(ReplaceFilter.FILTER_NAME, new ReplaceFilter()); | |||
filters.put(MergeFilter.FILTER_NAME, new MergeFilter()); | |||
filters.put(SplitFilter.FILTER_NAME, new SplitFilter()); | |||
filters.put(Base64EncoderFilter.FILTER_NAME, new Base64EncoderFilter()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide a base64 decoder too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebussieres, decoding a base64 string will return a byte array. If it's a binary object, reconstructing it might not be feasible. Do you suggest a return value type of byte[]
? The encoder was mainly targeted to strings with special characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
byte[] decodedBytes = Base64.getDecoder().decode(encodedString);
String decodedString = new String(decodedBytes);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case? I mean base64 encoding can be useful for putting in a link or showing a raw image but decoding seems pretty useless, if it's data from another server I'd expect it to be decoded before being provided to the template. Besides, new String(...) is loose and will probably cause problems on Windows machines, StandardCharsets.UTF_8 should be supplied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the decoder and it will try to return an UTF-8 String. If there's an exception, we'll throw it. No object casting will be done.
* | ||
* @author Silviu Vergoti | ||
*/ | ||
public class Base64EncoderFilter implements Filter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some documentation for the new filters too ?
https://github.com/PebbleTemplates/pebble/tree/master/docs/src/orchid/resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
Adding new filters.
Fixes issues #573 and #572.