[TYPO3-core] RFC #9474: Integrate OpenID authentication support to TYPO3
Ingo Renner
ingo at typo3.org
Sun Oct 19 23:51:45 CEST 2008
Hi Dmitry,
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()
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?
line 278:
smal typo: is -> if
line 279:
...there _is_ no...
line 344:
is it on purpose that there's nothong similar for the BE there?
line 16725, 16726:
typo: t3lub -> t3lib, decided -> decide
line 16728ff:
abbreviation: $authentificationInformation
line 16743ff, 16785ff:
abbreviation: $parentObject
(except for these two abbreviations I like that you used good variable
names accros the code!)
line 16759:
$available - what is available? milk at the local grocery store?
Can you find a better, more descriptive name? Maybe $openIdAvailable
will do?!
line 16799:
...returns _the_ user... back to -the- t3lib_userAuth...
line 16846:
$result -> $authenticated or even better $authenticationStatus
line 16896:
wouldn't be better to set the flag _after_ everything has actually been
inlcuded?
line 16898:
incomplete comment?
line 16902:
you're using PATH_SEPARATOR around that line, why not in front of '/Auth'?
line 16942:
I'd suggest to call the method getUserRecord()
line 16966:
What about this TODO item, from the text it seems quit important to me?!
line 17034, 17070:
move the else one line up
line 17213:
not needed as it's already in the dependencies section further down. If
someone's going to use this with a version lower than 4.3 it won't work
anyways as the HTTP stauts constants in t3lib_div have been used...
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.
best
Ingo
--
Ingo Renner
TYPO3 Core Developer, Release Manager TYPO3 4.2
More information about the TYPO3-team-core
mailing list