sort(<+|-><property>) vs. sort(<property>,<direction>)

[IllyaMoskvin]

According to the RQL specification, sort() requires a list of properties prepended with a + or -, not a property followed by a direction. The current parser does a great job of allowing this functionality, no complaints there, but the sort() description in QueryInterface.php is misleading. Something akin to the following might be better:

/**
 * @param string $fields,... List of fields, each prefixed with <+|->
 *
 * @return mixed
 */
public function sort(/* ... */);

...and here is one possible implementation:

/**
 * {@inheritdoc}
 */
public function sort(/* ... */)
{

    $fields = func_get_args();
    $out = array();

    // Note that + must be URL encoded as %2B
    foreach( $fields as $field ) {
        switch( $field[0] ) {
            case '+' : array_push($out, substr($field, 1) . ' ASC' ); break;
            case '-' : array_push($out, substr($field, 1) . ' DESC' ); break;
            default: throw new \Exception("Must specify valid sort order for field {$field}"); break;
        }
    }

    $out = implode( ', ', $out );

    // And just for debugging...
    print_r( $out );
    exit;

}

Personally, I like implementing sorting as sort($field, $direction = false), where $direction indicates whether the sorting should be reversed (i.e. DESC). This works great with the pipe/ajax functionality of the SmartTable module for AngularJS. (However, much like the suggested implementation described in QueryInterface.php, my implementation does not follow RQL spec.)

Here it is, if anyone out there is looking for more examples:

public function sort($field, $direction = false)
{
    // protected function wrapTable() turns objects.title into `objects`.title
    // It is meant for resolving ambiguity in joins by verifying against a list of allowed tables
    // e.g. foobar.title would not be allowed, b/c table `foobar` is not assoc. with the current entity
    $field = $this->wrapTable( $field );
    $direction = filter_var($direction, FILTER_VALIDATE_BOOLEAN);
    $direction = $direction ? 'DESC' : 'ASC';
    array_push( $this->order, "{$field} {$direction}" );
    // ...and then I retrieve the order with $queriable->get_order();
    // Of course, currently, I'm limited to sorting by one field at a time
}

I'm using PHP ActiveRecord in my project. I've written a QueryInterface implementation for it, which generates an $options['conditions'] array. I'd be happy to add it to the project as an example. I've never done a pull request before, but let me know if you are interested, and I'll figure out how to do it. Cheers!

[hairmare]

Hi Illya

Thanks for your comment. There are arguments to be made for both forms of the sort command. While the sort(<property>, <direction>) form makes for readable code the sort(<+|-><property>) allows for expressing multi-sorts in a non ambiguous way.

At the moment we are trying to support both syntaxes in our 1.x releases. We are still undecided whether that will still be the case in v2. The problem here is that we do not want make asc or desc reserved words as a backend might have fields.

Please consider a table with a name and desc field for the following example. How would a Queriable know differentiate second argument here?

sort(-name,desc)

In standard RQL this always expresses a multi-sort, however if both syntaxes are allowed, it becomes hard to tell what was originally intended.

We would be interested in integrating you PHP ActiveRecord based solution at some point in the future.

As the RegExp based approach used by 1.x version of the lib was not flexible enough, we have started a refactor to using doctrine/lexer. This refactor is already merged on develop and it also does away with the QueryInterface bit while replacing it with an AST and Visitor pattern. We will be releasing this as 2.0.0-alpha1 soonish.

Could you consider integrating PHP ActiveRecord against that work? We are currently refactoring the AST with the aim to create an AST that supports as many possible visitor implementations as possible.

[IllyaMoskvin]

Hi hairmare,

Thanks for the quick response! Coming from a SQL background, I'm inclined to suggest that it should not be necessary to worry about the possibility that desc or asc might be field names: they are identified as reserved words in MySQL, Transact-SQL, PostgreSQL, etc. However, I do understand that there's folks out there who have to work within the constraints of legacy databases, and although I'm not yet acquainted with NoSQL solutions, I'm guessing that they do not have these reserved words.

So, I don't have a concrete answer to this issue, except to posit that resolving the ambiguity should be left to the implementation (i.e. currently, whichever class implements QueryInterface). I do like the current approach of simply passing the function arguments to the QueryInterface, since it allows the implementation to have the final word, but I understand the need for a refactor to handle more complex syntax.

Admittedly, I haven't worked with an AST and Visitor pattern before, but I'd be happy to learn the ropes and re-write my PHP ActiveRecord implementation for it after the alpha is released!

Edit: Fixed links.

[hairmare]

Thanks for the input. I see that it should be up to the visitor to handle the possible ambiguity of desc and asc. Generally speaking it should be up to each visitor to handle reserved keywords since they will differ vastly across relational, legacy and NoSQL solutions.

Implementing the AST Visitor should not be much harder that writing a Queriable. It adds the benefit of making recursive queries possible. Basically your Visitor implements a visit method that gets passed the AST representation of the query string as a composition of objects. You then traverse this object tree using whatever means that make sense for your backend. My current visitor in src/Graviton/Rql/Visitor/MongoOdm.php is a bit hacky. I plan on replacing it with a cleaner solution while I refactor the AST.

During this refactor I will not be adding any special handling to asc and desc as the second param of a sort() call. It will then be up to each visitor to choose which strategy to implement and expose.

Let me know if this is ok for you going forward and I'll close this issue. Feel free to open a new one if you have any questions.