Alias and Autofields

[dcodadigital]

Avoids ambiguous field names if "latitude" or "longitude" are the set field column names.

Included autoFields so that the select fields are not over written if combined with another query.

[chris48s]

Hi @dcodadigital.

Your bug report is completely valid, so thanks for submitting it. I see what you are getting at with your attempted solution, but there are some problems with your patch. I can see from your profile that you are fairly new to this, so I'll run you through it.

• In principle, a conflict could occur on fields with names other than latitude and longitude. For example, if my columns are called lat and lon and I extend the query to join to another table with columns called lat and lon, I will encounter the same problem. You don't need those if statements at all. The code should just be:

$this->_config['latitudeColumn'] = $this->_table->alias() . '.' . $this->_config['latitudeColumn'];
$this->_config['longitudeColumn'] = $this->_table->alias() . '.' . $this->_config['longitudeColumn'];

This is a more generic solution.

• Aside from that, your solution works for Postgres, but it doesn't work in MySQL. The unit tests only tests passed in MySQL because you've added code in such a way that none of the conditions you've added are actually matched in the tests! This is why the coverage has decreased. See if you can work out what you need to do in order to make this work in both MySQL and Postgres (without introducing a SQL injection flaw, please).
• Your code has inconsistent whitespace and does not conform to the CakePHP coding standards, which is why the Travis build is failing. You can look at the Travis output to see what the problems are.
• You have added unnecessary whitespace. This makes diffs harder to understand.

Hints:

• Before commiting, run the unit tests using

vendor/bin/phpunit

and check against coding standards with

vendor/bin/phpcs -n -p --extensions=php --standard=vendor/cakephp/cakephp-codesniffer/CakePHP --ignore=vendor --ignore=docs --ignore=tests/bootstrap.php .

• You can set up Travis to run on a fork so if you are working on a project that supports different versions of PHP and different database engines like this one, set up travis on your fork to run all those checks before submitting a pull request upstream. It is easier than having to maintain all those different environments locally to ensure compatibility.

I'd like to include your patch, rather than just fixing the bug myself, so I look forward to receiving an updated PR from you.

[dcodadigital]

Hey @chris48s

Thanks for spending time to reply in such depth with some very quality pointers I'll certainly take on-board going forward.

That's a good point on the if statements they can just be aliased without them. I had to look at the query as I found out the aliases would not work within the query conditions as they were so ended up changing that slightly: -

$query->autoFields(true)->find('all')
->select(['distance' => $distance])
->bind(':earth_radius', $earthRadius, 'integer')
->bind(':latitude', $latitude, 'float')
->bind(':longitude', $longitude, 'float')->where([$distance . "<=" . $radius]);

I will have another go and submit another PR over this weekend and use proper unit testing. Thanks for your patience with me.

[chris48s]

OK, so you've figured out how to make it work, but I think your solution now introduces that SQL Injection flaw I talked about. $radius could be user supplied data, so we need to ensure it is sanitized before sending it to the database. There's probably several ways to deal with that, but for consistency with the rest of the code, I'd make a named parameter :radius, so we can rewrite that as:

$queryOptions = [
    'fields' => ['distance' => $distance],
    'order' => ['distance ASC'],
    'conditions' => ["$distance <= :radius"]
];
$query->find('all', $queryOptions)
->autoFields(true)
->bind(':earth_radius', $earthRadius, 'integer')
->bind(':latitude', $latitude, 'float')
->bind(':longitude', $longitude, 'float')
->bind(':radius', $radius, 'float');

You should now have what you need to submit a working (secure) patch.

I actually think CakePHP needs to be more explicit about the cases where it doesn't sanitize input as I think the docs don't make it clear enough that

'conditions' => ["$distance <=" => $radius]

is safe, but

'conditions' => ["$distance <= $radius"]

is not!

[chris48s]

Fixed in ab3c631 - closing