[TYPO3-core] RFC: fix feature request #568

[Michael Stucki]

Hi Wolfgang,

thanks for taking care of this!

One general notice:
When proposing such big changes, it would be great to send 1 original patch
and 1 which ignores the whitespace-changes. This would be much easier to
review (use "diff -wu").

>  1) $this->browserInfo is never really used (not in other classes too),
>     so I dropped it
>  2) changed every ereg* stuff to the preg equivalent
>  3) changed tests like "if ($test)" to "if (strlen($test))"
>  4) inserted an explicit "return false" where it was missing
>  5) moved the hook in 'whichDevice' to the top of the method as the
>     result is not used if the hook is called anyway (see below)

All fine.

>  1) the hook in 'whichDevice' is odd:
>         - there's a foreach but after the first found object return is
>           called

Right, that's a bug. It should return after the first match, but currently
it doesn't check if the function really matched.

BTW, please add a comment that this hook was requested by Michael Perkhofer
(michael at perkhofer.at) for his "wurfl" extension (though it doesn't use it
yet).

>         - the hook management is not created for this class but only
>           for this single method (compare with e.g. t3lib_tcemain)
> 
>      so I suggest the following:
> 
>     rename
>     ['SC_OPTIONS']['t3lib/class.t3lib_matchcondition.php'
['devices_class']
>     to
>     ['SC_OPTIONS']['t3lib/class.t3lib_matchcondition.php'
['matchConditionclass']

ACK. As it seems the hook is not used yet (but who knows if other extensions
are using it?). I think you can change this, that was my fault.

>   this allows me to add another hook in 'browserInfo' (I've written
>   a wrapper for phpSniff (LGPL) which is more sophisticated... I'll
>   release that soon as an extension)
> 
>   (.. and I don't like the naming of 'whichDevice_ext', why not simply
>      whichDevice as it's in its own class anyway, but ok...)
> 
>   So I would add a member variable "hookObjectsArr" and register the
>   hook class(es) there and use method_exists as it's done in e.g.
>   tcemain.
>   (the patch does not contain that, I've simply copied the other part
>   and renamed it to 'browserinfo_class', so this is really temporary)

OK.

>  2) how do I handle a hook, that can only contain a single reference
>    (like above) as there's no meaning in calling two methods after each
>    other, what's the general practice (besides not using a 'foreach' where
>    'list' is more logical)?

There are no "rules" for this atm. Maybe someone should propose a little
guideline?

>  3) is $this->browserInfo required somewhere else (I found no reference
>     to it in any other "core" class)?

Maybe Kasper knows.

>  request:
>     please test the attached patch (although I will change it in
>     response to your answers)

Will do that later. In any case you patch needs Kaspers OK because it
touches the TS parser which is critical...

Regards, michael
-- 
Use a newsreader! Check out
http://typo3.org/community/mailing-lists/use-a-news-reader/