[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