[TYPO3-core] RFC #9474: Integrate OpenID authentication support to TYPO3
Oliver Hader
oliver at typo3.org
Mon Oct 20 16:15:20 CEST 2008
Hi Ingo & Dmitry,
Ingo Renner schrieb:
> here's my review:
>
> line 34:
> please use names for the variables that tell what information they
> contain, even if that information is not needed at this point.
>
> line 47:
> why do you need to loop over the array to find an empty spot? How about
> a simple count()
I'd like to see the whole tx_openid_addToPalette() function being part
of t3lib_extMgm - I could do this refactoring and integration - only if
nobody else wants to... ;-)
> line 109:
> please do not add commented code to the core. BTW: Why do you need that
> special function to add the field instead of simply using the core
> functions?
Ok...
> line 278:
> smal typo: is -> if
>
> line 279:
> ...there _is_ no...
Ok, typos in the comment...
> line 344:
> is it on purpose that there's nothong similar for the BE there?
Hm, indeed it's a good question...
> line 16966:
> What about this TODO item, from the text it seems quit important to me?!
To me it also looks important to have the data stored in the database...
> so all in all I'd say 98% of the patch is great! Just fix these small
> remarks and we can get this baby in at last.
IMO the remaining formatting/naming issues are justifiable and can be
fixed easily.
olly
--
Oliver Hader
TYPO3 4.3 Release Manager
More information about the TYPO3-team-core
mailing list