Hi Martin,
On 2007-09-18 at 14:55:34 [+0200], Martin Edenhofer
Oliver Tappe schrieb:
[ ... ]
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?).
No, having read your mail I still think we are talking about the same thing ;-)
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.
Yes, that's the aim, but it is also a lot of work to do so (many lines of code would have to be changed). But if we see the chance, we should do it all in one big refactoring step.
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.
Hm, I do not seem to understand you. What do you mean by default objects? And why shouldn't one use $Self->{LogObject} in other perl objects? AFAICS, that kind of use is spread all over the place. What's the problem with it?
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.
Well, CheckParams() is not meant to check object "content" at all (as you are not allowed to look into objects by definition anyway). The more complex structures are only supported for hashes (not objects). It gets a bit complicated, as objects are in fact implemented as hashes, but CheckParams() is only intended to check the parameter hash given to a function. Think of it like this: after the invocation of CheckParams(), a method knows that all required paramteres have been passed in and (optionally) that they are of the correct type.
o) OO-Style; Only OO-Style. It would be not good to switch to other styles.
Ok, you have mentioned this before, but I dare to question the validity of the argument behind it: Perl is a hybrid programming language (like C++, but unlike Java and Smalltalk for instance) and it provides a procedural and an object-oriented interface, both of which are being used by OTRS. As an example, OTRS uses the functions print(), split(), join(), open() and close() in many places, all of which are part of perl's procedural interface. I perfectly understand that most of the time, an object-oriented interface (involving a class) is more apropriate, but *not* every time. Objects usually encapsulate some kind of state or complexity, but in this case there would be no state or complexity to encapsulate. Consider this: CheckParams($Params, sub { ... }) is so much more convenient than $Checker = Kernel::System::Params->new($Params); $Checker->Check(sub { ... }); which would be the standard way of doing it in OTRS style. I personally find this rather awkward, as it makes it rather difficult to grasp the meaning of the code. However, object-oriented interfaces provide an alternative solution when it comes to implementing functions: class methods (discussion follows below): Kernel::System::Params->Check($Params, sub { ... })
o) confess(): I have looked into your source code. I never have seen confess() in the OTRS framework bevor. Whats about this?
confess() is the standard way of requesting a die together with a stack crawl. It is provided by the Carp module. One could of course use $Self->{MainObject}->Die() instead, but that would depend on having an instance of MainObject available, which may not always be the case.
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.
Yep, Kernel/System/Params.pm would be correct another suggestion would be Kernel/System/MethodArgs.pm (which at least for me expresses the actual meaning of the module best).
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
Yes, standard OTRS style, which would require every method wanting to check its arguments to create (or already have access to) an object of the CheckParams class. As expressed above, I find that less clear (and clarity is what I like a lot ;-). The strangelooking invocation is a class method call (just like new()), i.e. "Kernel::System::Params->Check()" invokes the method Check() on the class Kernel::System::Params. Class methods are a common ocurrence in object oriented software development.
"$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', );"
Hey, but that is using a hash instead of a hash-ref, wasting performance along the way, as that hash would have to be copied during the invocation of the method (I remember Richard Jelinek mentioning that). Using an anonymous subroutine (sub { ... }) instead of a hash-ref makes it possible to avoid the performance hit of generating the hash at all (like for the case when the parameter checking has been disabled via SysConfig).
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.
Yes, you are right, it would be good to avoid the use of regexps and that can be done easily (replacing regexps by substr()).
I mean something like e. g.
'TimeObject' => { Operation => '!', Type => 'Class', Match => 'Kernel::System::Time', },
IMO this would be faster and need less cpu time.
Yes, that certainly would be another way to avoid the regexps. I had outlined the very compact declarative interface (using just one string) such that every check could be contained on one line, making the code easier to read. But maybe it would indeed be better to switch to a more attribute-like declaration like you have suggested. I don't know, really - is there anybody else with a view on this?
o Maybe we need something like Kernel::System::Main->Die().
Yes, I'd like that. I personally would favor a *function* named Croak(), automatically exported by Kernel::System::Basics, but that is probably just me ...
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? :-)
I can't really see the difference between these two. Currently, CheckParams() can offer both, it just depends on what you ask for: Kernel::System::Params->Check($Self, sub { 'ConfigObject' => '!', 'DBObject' => '!', 'LogObject' => '!', 'MainObject' => '!', 'TimeObject' => '!', # 'Filename' => '!', 'IsUTF8' => '?', }); would do just that: check only the existence of the required framework objects and check the functional parameters, too (the hashmark just separating these two for visual clarity). Notwithstanding any further discussion on this list, I think all of this would be a fine topic to talk about over dinner during our meeting next week ;-) cheers, Oliver -- ((otrs)) :: OTRS GmbH :: Europaring 4 :: D - 94315 Straubing Fon: +49 (0) 9421 56818-0 :: Fax: +49 (0) 9421 56818-18 http://www.otrs.com/ :: Communication with success!