kolibri-discuss team mailing list archive

[Frode Danielsen <frode@xxxxxxx>]

Review: Needs Fixing
I've read through the changed functionality of this branch, and it almost looks good to go! One possible critical bug though, as far as I can see:
- ModelProxy:100 sends $innerModel to propagateKey(), but $innerModel can't be anything but undefined. Seems to be a leftover after rewriting the encapsulating foreach.

Another thing that should be fixed is how ModelInterceptor iterates through request parameters. This will not function as intended when merging with trunk. You should iterate through the public $params property of the Request object from now on instead of iterating over the Request object itself.

Apart from those two fixes to be made, I only have some minor questions:
- Does empty() perform an implicit isset()? Otherwise it seems to me ModelProxy could raise PHP warnings for the !empty($model->original) checks on fresh model objects (not persisted to database yet).
- DatabaseConnection lines 206-215 does a lot of legwork to wrap scalar and NULL values in an array, while this could be solved simply by casting $params to an array. This should solve both scalar and NULL values, as per spec: http://www.php.net/manual/en/language.types.array.php#language.types.array.casting

And of course, I have some tiny comments and general suggestions as well:
- The comment for ModelAware should specify that populating $model from session only happens after a _failed_ validation.
- ModelProxy:13 lacks a * in the phpdoc comment. TextMate bugs me with highlighting it
- ...and it's "available", not "availible". I keep coming across that :)

But all in all, as said, this looks pretty good!
-- 
https://code.launchpad.net/~asteinlein/kolibri/validation-improved/+merge/5355
Your team Kolibri Discuss is subscribed to branch lp:kolibri.