Hi Oliver,
Oliver Tappe schrieb:
> attached is a function I have found useful in some of the OTRS-modules I
> worked on recently. It is called CheckParams() and its purpose is to offer an
> intuitive and flexible way of checking the params-hash that has been passed
> into a method.
>
> Currently, many methods contain code like this:
>
> # check all needed objects
> foreach (qw(ConfigObject LogObject DBObject TimeObject MainObject)) {
> die "Got no $_" if (!$Self->{$_});
> }
>
> plus more checks for additional parameters that must be passed in via
> the %Params hash.
>
> When using CheckParams, the code looks like this:
>
> # check all needed objects
> CheckParams($Self, sub {
> 'ConfigObject' => '!Class=Kernel::Config',
> 'DBObject' => '!Class=Kernel::System::DB',
> 'LogObject' => '!Class=Kernel::System::Log',
> 'MainObject' => '!Class=Kernel::System::Main',
> 'TimeObject' => '!Class=Kernel::System::Time',
> });
>
> As you can see, you pass CheckParam() the hash-ref that shall be checked and
> a
> specification (for performance reasons this should always be an anonymous
> subroutine). In the specification, you simply declare the supported
> parameters, whether or not they are required (!) or optional (?) or if they
> have to be of a specific class (Class=...).
>
> CheckParams() supports checking of more complex data structures, too:
>
> CheckParams($Params, sub {
> 'GeneralAttrs' => {
> 'From' => '!',
> 'To' => '!',
> },
> 'JobAttrs' => {
> 'Creator' => '!',
> 'Changes' => [
> {
> 'Action' => 'm{^(create|change|delete|sync)$}',
> 'Path' => '!',
> 'Content' => '?',
> }
> ],
> }
> });
>
> This code snippet makes sure that the given $Params-hash contains two
> elements
> (GeneralAttrs & JobAttrs), each of which has to follow the specified
> substructures.
>
> As I believe this function could be useful throughout OTRS (now that we have
> the 2.2-branch), I'd like to add it to the framework. I can see two ways of
> doing that:
>
> 1.) non-OO style: add some kind of helper module (e.g. Utils.pm) and add
> CheckParams() as a *function* to that.
> 2.) OO style: add a new module Utils/Params.pm which contains a
> *class-method*
> Check(), such that you'd have to invoke it like this
> Utils::Params->Check($Params, sub {
> ...
> });
>
> So what do you think? Do you like the idea in general at all? If so, how
> should that functionality be integrated (OO- or non-OO-style)?
>
> Please fire any suggestions or questions to this list or directly my way.
-=> Yes, 'die "Got no $_" if (!$Self->{$_});' the die() is not a option. ;)
Bit I see different things to integrate it into the current OTRS
framework (may be we are talking about two different things?).
Anyway, here my points:
o) Keep it clean:
If we have a check method in the framework, we should replace any
other checks from the whole framework with the new check to keep it clean.
o) Check it every time?/Performance Issue:
"# check all needed objects" - For normal framework, it should not be
necessary to check the default objects. The framework should only
deliver existing objects. You never should use e. g. $Self->{LogObject}
for other perl objects. I also would see a really performance issue if
we would check the object content every time. -=> So for the OTRS
framework I would expect to not need to check the base objects.
o) OO-Style;
Only OO-Style. It would be not good to switch to other styles.
o) confess():
I have looked into your source code. I never have seen confess() in
the OTRS framework bevor. Whats about this?
o) Filename:
Utils/Params.pm is not on the current directory style.
Kernel/System/Params.pm or to extend the existing
Kernel/System/CheckItem.pm would sounds a little bit better to me.
o) Utils::Params->Check($Params, sub { ... }); looks a little bit
strange to me. I would suggest something like "$Self->{ParamsObject}"
oder "$Self->{CheckObject}". And a methode like
"$Self->{ParamsObject}->Check(
'ConfigObject' => '!Class=Kernel::Config',
'DBObject' => '!Class=Kernel::System::DB',
'LogObject' => '!Class=Kernel::System::Log',
'MainObject' => '!Class=Kernel::System::Main',
'TimeObject' => '!Class=Kernel::System::Time',
);"
o) One other thing would be to define more via attributes. E. g.
'TimeObject' => '!Class=Kernel::System::Time',
Is IMO not so good because you need to use some cpu time to use
a regexp for to find out what "!Class=" is.
I mean something like e. g.
'TimeObject' => {
Operation => '!',
Type => 'Class',
Match => 'Kernel::System::Time',
},
IMO this would be faster and need less cpu time.
o Maybe we need something like Kernel::System::Main->Die().
So back to the two different things:
IMO we need only a exists-check for objects (not content related) but we
could use a new Check() for param validation of e. g. strings or other
content.
What do you think? :-)
Greetings,
-Martin
> ------------------------------------------------------------------------
>
> =item CheckParams()
>
> Utility function that can be used by any method that accepts param-hashes
> to check if the given parameters actually match the expectations.
>
> Each individual parameter has a specification that describes the expectation
> that the calling function has towards this param. The following specifications
> are supported:
>
> * '!' - the parameter is required
> * '?' - the parameter is optional
> * 'm{regex}' - the parameter must match the given regex
> * '!Class=...' - the parameter is required and must be an object of the given
> class
> * '?Class=...' - if the parameter has been given, it must be an object of the
> given class
>
> The function will confess for any unknown, missing, or non-matching param.
>
> =cut
>
> sub CheckParams
> {
> my $Params = shift or confess('need to pass in params-hashref!');
> my $ParamsSpec = shift or confess('need to pass in params-spec-hashref
> (or -sub)!');
>
> # TODO: allow to switch off this function via configuration in production
> # environments, as it is rather heavy
>
> # fetch param-spec from function, if that has been given:
> if (ref($ParamsSpec) eq 'CODE') {
> $ParamsSpec = $ParamsSpec->();
> }
>
> # print a warning for any unknown parameters that have been given:
> my @UnknownParams
> = grep { !exists $ParamsSpec->{$_}; }
> keys %$Params;
> if (@UnknownParams) {
> my $UnknownParamsStr = join ',', @UnknownParams;
> confess("Enocuntered unknown params: '$UnknownParamsStr'!\n");
> }
>
> # check if all required params have been specified:
> foreach my $Param (keys %$ParamsSpec) {
> my $Spec = $ParamsSpec->{$Param};
> if (ref($Spec) eq 'HASH') {
> # Handle nested specs by recursion:
> my $SubParams = $Params->{$Param};
> if (!defined $SubParams) {
> confess("Required param '$Param' is missing!");
> }
> CheckParams($SubParams, $Spec);
> }
> elsif (ref($Spec) eq 'ARRAY') {
> # Handle nested spec arrays by looped recursion:
> my $SubParams = $Params->{$Param};
> if (!defined $SubParams) {
> confess("Required param '$Param' is missing!");
> }
> elsif (ref($SubParams) ne 'ARRAY') {
> confess("Value for param '$Param' must be an array-ref!");
> }
> foreach my $SubParam (@$SubParams) {
> CheckParams($SubParam, $Spec->[0]);
> }
> }
> elsif ($Spec eq '!') {
> # required parameter:
> if (!exists $Params->{$Param}) {
> confess("Required param '$Param' is missing!");
> }
> }
> elsif ($Spec =~ m{^\!Class=(.+)$}i) {
> my $Class = $1;
> # required parameter ...
> if (!exists $Params->{$Param}) {
> confess("Required param '$Param' is missing!");
> }
> # ... of specific class
> if (!$Params->{$Param}->isa($Class)) {
> confess("Param '$Param' is not a '$Class', but that is
> required!");
> }
> }
> elsif ($Spec eq '?') {
> # optional parameter - nothing to do
> }
> elsif ($Spec =~ m{^\?Class=(.+)$}i) {
> my $Class = $1;
> # optional parameter ...
> if (exists $Params->{$Param}) {
> # ... of specific class
> if (!$Params->{$Param}->isa($Class)) {
> confess("Param '$Param' is not a '$Class', but that is
> required!");
> }
> }
> }
> elsif ($Spec =~ m{^m{(.+)}$}) {
> # try to match given regex:
> my $Regex = $1;
> my $Value = $Params->{$Param};
> if ($Value !~ m{$Regex}) {
> confess("Required param '$Param' isn't matching regex
> '$Regex' (given value was '$Value')!");
> }
> }
> else {
> # complain about unknown spec:
> confess("Unknown param-spec '$Spec' encountered!");
> }
> }
>
> return scalar 1;
> }
_______________________________________________
OTRS mailing list: dev - Webpage: http://otrs.org/
Archive: http://lists.otrs.org/pipermail/dev
To unsubscribe: http://lists.otrs.org/cgi-bin/listinfo/dev
