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

[Bernhard Kraft]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Wolfgang Klinger wrote:


First to say:

If you create patches which leave much of the code untouched but indent it to another level
you should use the "-w" switch so indenting is not compared (This makes it much more easier
to follow your changes) - of course never apply such a patch. Just for looking at the changes.
(switches should be: -rwu)

Please send again such a easier to read patch and I will look on it, test this one (send a new
one with indendting respected if you changed something in between) and send you my opinion.


>  1) $this->browserInfo is never really used (not in other classes too),
>     so I dropped it

Ok with it.

>  2) changed every ereg* stuff to the preg equivalent

Will have a look at the changes one ... but currently hard to find.

>  3) changed tests like "if ($test)" to "if (strlen($test))"

Fine with that if also (string)"0" should return true for that (is not everywhere
the case in T3. i.e. if you check a TS value it might have also been set to (string)"0" which
means "false". i.e: "stdWrap.insertData = 0" )

>  4) inserted an explicit "return false" where it was missing

Ok.


>  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)

Will look on that.

>  questionable:
>  1) the hook in '
' is odd:
>         - there's a foreach but after the first found object return is
>           called
>         - the hook management is not created for this class but only 
>           for this single method (compare with e.g. t3lib_tcemain)

???

You have 't3lib/class.t3lib_matchcondition.php' definig the file in which the hook
is found and "devices_class" as the name of the hook.
in t3lib_tcemain.php you have "t3lib/class.t3lib_tcemain.php" as file name and
"preProcessDatamapClass" as hook name for example.

Whats wrong here ?

>      so I suggest the following:
> 
>     rename ['SC_OPTIONS']['t3lib/class.t3lib_matchcondition.php']['devices_class']
>     to ['SC_OPTIONS']['t3lib/class.t3lib_matchcondition.php']['matchConditionclass']

We can't simply rename a hook. at least we would have to provide backwards compatibility
for at least one version (so the author of the extensions which use that hook can take
care of it)

If I understand you correct you would like to have only ONE ['matchConditionClass'] array
for the whole class ?? Or am I misunderstanding something ?


>   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)

Just use:

    if (is_array($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_matchcondition.php']['browserInfoClass'])) {


>   (.. and I don't like the naming of 'whichDevice_ext', why not simply
>      whichDevice as it's in its own class anyway, but ok...)

We can't change that either. Because of compatibility.

If you provide a backwards compatible version I would be fine with it. And let's remove the
backwards compatibility let's say in 4.2.0 or so

>   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 hookObjectsArr in the mentioned example are always local variables !

>  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)?

Stay with the coding guidelines and simply do a "return" or "break" after the
first valid hook-object (or the first one which returned a valid value).

Using only a simple variable instead of an array would bring up other questions
why the standard is not kept in this place.

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

I'm fine with making it a local variable.


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

Will do that as soon as you send a "-w" diff



greets,
Bernhard
- --
- ----------------------------------------------------------------------
"Freiheit ist immer auch die Freiheit des Andersdenkenden"
Rosa Luxemburg, 1871 - 1919
- ----------------------------------------------------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFDz3iFIl4dkVkDMFkRAqhlAJ49kd0u8VKZ3o5qQc6XxA8roFmsngCfblba
CTYjo8lFr+MrkibS0ICnR4s=
=hYQB
-----END PGP SIGNATURE-----