Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

TreeBehavior: Changed $Model->alias . '.' . $field to $Model->escapeField($field) #728

Merged
merged 2 commits into from Aug 2, 2012

Conversation

Projects
None yet
3 participants
Contributor

boast commented Jul 21, 2012

Changed all string concatenations like $Model->alias . '.' . $field to the more consistent $Model->escapeField($field).

No TreeBehavior-TestCases fail after changes.

Changed all string concatenations like $Model->alias . '.' . $field t…
…o the more consistent $Model->escapeField($field).

@ADmad ADmad and 2 others commented on an outdated diff Jul 21, 2012

lib/Cake/Model/Behavior/TreeBehavior.php
@@ -308,7 +308,7 @@ public function children(Model $Model, $id = null, $direct = false, $fields = nu
$recursive = $overrideRecursive;
}
if (!$order) {
- $order = $Model->alias . '.' . $left . ' asc';
+ $order = "{$Model->escapeField($left)} asc";
@ADmad

ADmad Jul 21, 2012

Member

Using functions inside strings this way is bad.

@boast

boast Jul 21, 2012

Contributor

You're probably right, it's not the best way. But as I went through the Code I've found ~5 different ways how strings (especially those Table.Field cases) are concatenated, so I decided to use one way pretty consistently: double quotes with all PHP-(Return-)Values in curly braces.

In other programming languages you can't really tell and you don't care if a value is computed ($this->compute()) or just accessed ($this->var), you just call it 'query'. That was my intention.

In addition, I think that's exactly why we have the Model::escapeField()method.

@markstory

markstory Jul 22, 2012

Owner

You could always write it as $Model->escapeField($left) . ' asc';. That would be less prone to maintenance errors in the future.

@ghost ghost assigned markstory Jul 25, 2012

Owner

markstory commented Jul 25, 2012

This looks good now, I'll get it merged in.

markstory added a commit that referenced this pull request Aug 2, 2012

Merge pull request #728 from boast/patch-2
TreeBehavior: Changed $Model->alias . '.' . $field to $Model->escapeField($field)

@markstory markstory merged commit 2728c62 into cakephp:master Aug 2, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment