Announcement

Symphony's issue tracker has been moved to Github.

Issues are displayed here for reference only and cannot be created or edited.

Browse

Closed#622: Symphony mistakenly replaces AND by OR in filter SQL

When filtering publish entries via the URL, e. g. /symphony/publish/entries/?filter=datetime:2011-04, Symphony will replace all AND by OR statements based on this code in content.publish.php, line 91 to 95:

if (!is_null($where)) {
    $where = str_replace('AND', 'OR', $where); // multiple fields need to be OR
    $where = trim($where);
    $where = ' AND (' . substr($where, 2, strlen($where)) . ')'; // replace leading OR with AND
}

This breaks filtering by Date and Time fields which use BETWEEN x AND y in their filter syntax. Symphony mistakenly converts this to BETWEEN x OR y which is not a valid SQL statement.

It also breaks any SQL that needs to add more than one "AND":

$this->_key++;
$joins .= "
    LEFT JOIN
        `sym_entries_data_{$id}` AS t{$id}_{$this->_key}
        ON (e.id = t{$id}_{$this->_key}.entry_id)
";
$where .= "
    AND (
        t{$id}_{$this->_key}.entry_id IS NOT NULL
        AND (
            ".implode(($andOperation ? ' AND ' : ' OR '), $w)."
        )
    )
";

It replaces second AND to OR, creating hard to discover errors :(.

Why not store $where for each field in temporary array, and then just implode it? Are there any fields that do need to access existing $where and modify it?

There is hackish workaround: use and instead of AND (lowercase vs uppercase ;)).

I think I remember writing this code. I'll need to go through the commit history, although it might have been before the move to Github. I can't for the life of me remember what it does or why it is there...

"Multiple fields" in the code comment. I don't recall the backend filtering URL syntax allowing multiple fields. Unless that was my code change... to allow it.

Ok, I remember why I added this now. I was building a backend search (before Publish Filtering) where I needed to filter several fields by the same value. The syntax became:

?filter=title,body,author:regexp:Nick

The code then manipulates the queries so that they become "OR" (match regexp in Title, Body or Author fields).

This idea never got beyond the project, but the code went into the core. It is not a documented feature so I think it can safely be removed.

A neater future implementation for backend filtering would be to allow multiple field filters like I do on the REST API extension:

?filter[title]=Nick&filter[body]=regexp:Nick&filter[author]=Nick

So I would suggest:

  • we remove this functionality in the next release, it's evidently buggy
  • propose to the team that a neater implementation is done using arrays, but one that remains backwards compatible

@nickdunn Will removing the offending lines for 2.2.2 break the Publish Filtering extension?

I like the idea of using a proper URL array for filtering, it makes sense, is more powerful and less hacky. From what I can see the current implementation also touches on the prepopulation of fields and the fetching of associated entry counts too.

If I'm not mistaken, only one field can be prepopulated at the moment, but I think it filtering was moving to arrays, the side effect would prepopulation would also be able to work on multiple fields, which is a big win!

I'm unsure what sort of effect it would have on the associated entry count, but I think it might potentially mean that you could get a count of associated entries where fieldA = foo and fieldB = bar. Again, another massive win.

@nickdunn Will removing the offending lines for 2.2.2 break the Publish Filtering extension?

I don't think so. This syntax of being able to pass multiple filters in the form

?filter=title,body,author:regexp:Nick

is not supported by any extension to my knowledge. It was added by me years ago and is not used by Publish Filtering. The PF extension just filters a single field:

?filter=title:Nick+Dunn

So moving to a ?filter[field]=value syntax would break Publish Filtering, but I'd update it to work.

From what I can see the current implementation also touches on the prepopulation of fields and the fetching of associated entry counts too.

Agreed, but subtle differences. It doesn't affect the population of entry counts to my knowledge. The passing of a field was added back in the day of a Section Link so that clicking on the column count would then filter the child section by the parent ID, showing that number of entries. So if the syntax is changed then these extensions would need to change. To this end, I think any update needs to remain backwards compatible. We will need both ?filter=fieldname:value as well as ?filter[fieldname]=value, otherwise any field that hardcodes the former syntax (Select Box Link etc) would be broken.

We could update the prepopulate code at the same time, but I'm not convinced we need to just yet. It will increase the scope for bugs, which we should avoid.

I'm unsure what sort of effect it would have on the associated entry count, but I think it might potentially mean that you could get a count of associated entries where fieldA = foo and fieldB = bar. Again, another massive win.

Not sure I understand this. Could you elaborate? I don't think the filter affects the child entry counts, it just filters the parent section itself.

So in short:

  • we must remove this bad code for the upcoming Symphony release (won't break anything) (the bug)
  • we should implement filter[handle] syntax in addition to the existing filter=handle syntax
  • we could look at changing the prepopulate too, but isn't high priority

Righto, so I've removed the offending lines in this commit

We can look at moving to the filter[handle] syntax separately.

This issue is closed.

Symphony • Open Source XSLT CMS

Server Requirements

  • PHP 5.3-5.6 or 7.0-7.3
  • PHP's LibXML module, with the XSLT extension enabled (--with-xsl)
  • MySQL 5.5 or above
  • An Apache or Litespeed webserver
  • Apache's mod_rewrite module or equivalent

Compatible Hosts

Sign in

Login details