make the censor function customizable #1729
Conversation
lib/Dancer2/Core/Error.pm
Outdated
| For example, using L<Data::Censor>. | ||
|
|
||
| # in config.yml | ||
| error_censor: MyApp::Censor::censor |
There was a problem hiding this comment.
error_censor is not the catchiest name I ever came with, but... I guess it's descriptive? If anyone has a better name and/or a better location in the config, please speak up!
lib/Dancer2/Core/Error.pm
Outdated
| The function to use to censor error messages. By default it uses the | ||
| C<_censor> function of this package, but it can be configured via the | ||
| app setting 'error_censor'. If provided, C<error_censor> has to be | ||
| the fully qualified name of the censor function to use. That function is |
There was a problem hiding this comment.
Taking in a function name feels to me the most versatile way to go about it.
There was a problem hiding this comment.
As per my comment further up, if it could take an object, then you could pass in your own pre-configured Data::Censor (or other censoring) object with your desired settings (which fields to censor, what to replace with, custom per-field callbacks etc), which feels like it would be more flexible and powerful.
Maybe it could take either an object (which is expected to have a censor() method that takes input and returns the censored form) or a string containing the fully-qualified name of a function to call?
EDIT: That said, you've documented a good way of writing your own wrapper using Data::Censor to do pretty much exactly that. I should read the whole thing before opening my mouth :)
There was a problem hiding this comment.
For ease of config, we could do
# a string? It's a function
error_censor: MyApp::censor
# a hashref? A class that will have the method `censor` with its arguments for `new`
error_censor:
Data::Censor:
some_arg: 1
some_other_arg: banana
That might be nice sugar, as most people will only need the flexibility of option 2, and if they want moar there is still option 1 to do whatever the heck they want. :-)
lib/Dancer2/Core/Error.pm
Outdated
| my $self = shift; | ||
|
|
||
| my $custom = $self->has_app && $self->app->setting('error_censor') | ||
| or return \&_censor; |
There was a problem hiding this comment.
Related to #530, do we want to change our built-in _censor for Data::Censor directly? Is the code reduction worth the new dependency?
There was a problem hiding this comment.
Obviously I'd be more than happy to see Data::Censor being used (and it would reduce a little code, and Data::Censor itself has no further dependencies to worry about) but I don't think there's a massive benefit either way.
What I think would be valuable would be to be able to configure the given censoring solution, which isn't really possible here as far as I can see. If the censor attribute could be given an object that has a censor function, then you could instantiate your own Data::Censor object with your desired settings (e.g. custom lists of sensitive fields, custom replacements, callbacks per field etc) then pass it to the censor attribute to be used.
If we did switch to depending on Data::Censor and using it as the default censoring method, then maybe we could provide easy config settings to help configure it - but being able to pass in your own pre-configured object is probably enough?
There was a problem hiding this comment.
What I think would be valuable would be to be able to configure the given censoring solution, which isn't really possible here as far as I can see.
Oh, it totally is. This is why I set it up to take a function; so that one could then implement it whatever way they desire. The example included in the POD set up a Data::Censor object using a static config. If one wants to get dynamic, one could have something like:
package MyApp; # main App module
use Dancer2;
sub censor {
state $object = Data::Censor->new( settings->{data_censor_args}->%* );
$object->censor(@_);
}
related to #530
|
👍 as this currently stands. I'll take another look if we make |
|
I'll bring in the config change to take an object, and make Data::Censor the default. The new patch should come in soon! |
|
Heh heh. Data::Censor will need 2 little tweaks to work with D2. I have the patch, will try to send tonight. |
|
There we go. Once more thing punted out of core! \o/ |
|
👍 Really happy with how this turned out. What do you think @bigpresh ? |
related to #530