Encryption #2135
Encryption #2135
Conversation
|
You're crushing it tonight @jim-parry, keep it up :) |
system/Encryption/Encryption.php
Outdated
| */ | ||
| public function __construct($params = null) | ||
| { | ||
| $this->config = array_merge($this->default, (array) new \Config\Encryption()); |
There was a problem hiding this comment.
The config instance should be passed in. Makes it easier to test and customize, instead of forcing the dependency creation inside the constructor.
system/Encryption/Encryption.php
Outdated
| $params = ['driver' => $params]; | ||
| } | ||
|
|
||
| $params = $this->properParams($params); |
There was a problem hiding this comment.
I'm a bit confused why we're keeping an instance of $params here since they are set to $this->config in properParams()?
Seems cleaner to do $this->config = $this->properparams($params); here and not have properParams set it to me.
| } | ||
|
|
||
| // determine what is installed | ||
| $this->handlers = [ |
There was a problem hiding this comment.
This way of checking doesn't support users creating their own handlers, does it? Is that something we should allow for this? Part of me says yes, because if they're on 7.3+ and the framework is still 7.2, they could write one to take advantage of new features.
There was a problem hiding this comment.
it wasn't the intent that users create their own drivers for use here, but that we might provide several alternatives
| * @param string $str | ||
| * @return integer | ||
| */ | ||
| protected static function strlen($str) |
There was a problem hiding this comment.
Why do these methods exist in the different classes? I don't see them being used anywhere.
There was a problem hiding this comment.
they are used in the extending handlers, eg OpenSSLHandler
|
@lonnieezell No apologies needed - appreciate the feedback. |
|
I don't know what has gone wrong with the travis build, but definitely something serious :( |
|
Found & fixed the root problem with my travis build - messing with Services:logger. |
|
@lonnieezell Looking better - simplified, tested & hopefully addressing the concerns you raised :) |
|
@jim-parry Looks good to me. Merge at your discretion. |
Barebones encryption module - OpenSSL only.
This should satisy those that want "something" built in, while not offending those who don't want "anything" built-in.
libsodium support can be added once it gets bundled with our minimum version, and indeed the module is architected to make it easy to add any other encryption handlers.