[ORM] SearchFilter fatal error, call to method on non-object (i.e. no sanity check)

[patricknelson]

Affects all current branches. The following code generates a fatal error if you attempt to filter by a field such as one which you might find on a ManManyList via a $many_many_extraFields value:

( ! ) Fatal error: Call to a member function setValue() on a non-object in framework/search/filters/SearchFilter.php on line 195

For example, the extra field could be SortOrder and you might want to get something from a ManyManyList which may have a SortOrder value higher than 2, like this:

// This is a ManyManyList taken from some relation, somewhere off in the ether.
$list->filter(["SortOrder:GreaterThan" => 2])->toArray();

This fails because ->dbObject() (here: https://github.com/silverstripe/silverstripe-framework/blob/3/model/DataObject.php#L2948) doesn't always return an object, it can sometimes return null (albeit it's not explicitly defined in the docblock like it should be, but I digress). Either way, I'd highly suggest doing a quick sanity check in this ->getDbFormattedValue() method to prevent errors like these.

Affected code:

https://github.com/silverstripe/silverstripe-framework/blob/3/search/filters/SearchFilter.php#L199

    public function getDbFormattedValue() {
        // SRM: This code finds the table where the field named $this->name lives
        // Todo: move to somewhere more appropriate, such as DataMapper, the magical class-to-be?
        $candidateClass = $this->model;
        $dbField = singleton($this->model)->dbObject($this->name);
        $dbField->setValue($this->value);
        return $dbField->RAW();
    }

Recommended solution:

    public function getDbFormattedValue() {
        // SRM: This code finds the table where the field named $this->name lives
        // Todo: move to somewhere more appropriate, such as DataMapper, the magical class-to-be?
        $dbField = singleton($this->model)->dbObject($this->name);
        if (!$dbField) return $this->value;
        $dbField->setValue($this->value);
        return $dbField->RAW();
    }

I've not checked to see if this affects any other areas of the framework which may be depending on an object coming out of ->dbObject(). If this is a common (erroneous) assumption, it may be better to return a generic type, like Varchar or something.

[patricknelson]

Also, this may also be a good indication that $many_many_extraFields should be supported by ->dbObject() (on DataObject). So there are three issues here in one:

1. Sanity check here in SearchFilter->getDbFormattedValue()
2. Support for $many_many_extraFields in DataObject->dbObject()
3. Failover to a generic Varchar to encapsulate the value from the DataObject's result from ->getField($fieldName) and then return that just in case elsewhere in the framework someone attempts to fetch an object from ->dbObject().

Personally I don't like this last option since it sort of says "Uh yeah, uh ... sure I have that field! Uh here yah go!"

[tractorcow]

I'm confused by the existing code... we cast a DBField, but then just get the raw value straight back out of it? In which cases does that fix or protect against anything?

In the age of parameterised queries, I would guess that we might be fine with a simple return $this->value;.

[tractorcow]

Ok, it's all to do with DBField::setValue doing cleanup on entry. :P E.g. SS_DateField doing cleanup of raw strings into nice clean DB safe dates.

In this case, I guess you'd still need that for many_many_extrafields?

[tractorcow]

Ok, this is a possible place to look...

apparently SearchFilter::apply is passed two parameters, the DataQuery as the first, and the DataList as the second.

If you added $list to the second parameter, you could theoretically check for extra casting helpers (since this list would be a ManyManyList in some cases).

It would be a breaking API though, but doable. Just update all the SearchFilter methods to take both as parameters.

For 3.x we could do the fix you proposed, @patricknelson .

[tractorcow]

Also, some search filters are just doing getValue() when they should be using getDbFormattedValue() :D

[patricknelson]

I have no clue what you're talking about with SearchFilter::apply but I do agree with you on the assesment of simply returning $this->value on the SearchFilter instance (no need for Varchar encapsulation nonsense I mentioned).

My attempt to workaround this without making yet another modification to core (due to the fact that I still need to upgrade our site and I've already get a pile of PR's out there that I've yet to update or get around to or yet to be merged, etc). You know me -- I've got many issues that require PR's that I've yet to get to (part of why my company is poking you guys for more formal help, by the way :wink:). So: Funny you should say that about parameterized queries, though. I'm currently working around the tangled mess of interrelated dependencies of GridField and this GridFieldSortableRows object (unrelated, but it's here: https://github.com/UndefinedOffset/SortableGridField/blob/master/code/forms/GridFieldSortableRows.php#L207) which goes right back and tries to get the record off the GridField by proxy of a Form. All that since the object itself (as you can see there) uses Raw Queries to work around the fact that you cannot really touch extra fields... due to a gap in the framework :disappointed:

[patricknelson]

So @tractorcow you'll find probably about 4 or 5 instances in our code where I spitefully write:

$form = new Form(new Controller(), "whocares", $obj->getCMSFields(), new FieldList());

Haha... ugh.

[tractorcow]

I'm in my own little world...

let me put it his way.

/**
     * Applies a comparison filter to the query
     * Handles SQL escaping for both numeric and string values
     *
     * @param DataQuery $query
     * @param DataList $list
     * @return $this|DataQuery
     */
    protected function applyOne(DataQuery $query, DataList $list) {
        $this->model = $query->applyRelation($this->relation);

        $predicate = sprintf("%s %s ?", $this->getDbName(), $this->getOperator());
        return $query->where(array(
            $predicate => $this->getDbFormattedValue($list)
        ));
    }

    /**
     * Return the value of the field as processed by the DBField class
     *
     * @param DataList $list
     * @return string
     */
    public function getDbFormattedValue($list) {
        // SRM: This code finds the table where the field named $this->name lives
        // Todo: move to somewhere more appropriate, such as DataMapper, the magical class-to-be?
        $candidateClass = $this->model;
        $dbField = singleton($this->model)->dbObject($this->name);
        if(!$dbField) {
            if($list instanceof ManyManyList) {
                $extras = $list->getExtraFields();
                if(isset($extras[$this->name])) {
                    $dbField = Object::create($extras[$this->name]);
                }
            }
        }
        if(!$dbField) {
            return $this->value;
        }
        $dbField->setValue($this->value);
        return $dbField->RAW();
    }

[patricknelson]

I see what you mean. You originally lost me at ->apply() - so you meant ->applyOne(). Plus passing a list like that so deep into the matrix seems a bit fucky* to me. Instead, maybe it makes more sense for ->dbObject() to detect this for you, since that's what you're after. The database object representing that field, regardless of where it really lives. I suppose maybe the difficulty here is that (if I recall correctly somewhere in the dark corners of my SilverStripe knowledge) that these extra fields are only present on certain objects AFTER they're fetched from a ManyManyList which is why I can tell you're passing that list down the stack.

So -- why can't you just get those extra fields off the model, which you already have? Doesn't that singleton have access to it's own extra fields in many/many relations?

*Yes I misspelled that but didn't bother to fix it because it fits :smile:

[tractorcow]

Plus passing a list like that so deep into the matrix seems a bit fucky* to me

I think it's ok, search filters are MENT to be used on DataLists, so it's probably ok for the context to be there.

[tractorcow]

So -- why can't you just get those extra fields off the model, which you already have?

Because extra fields aren't a component of the model; They are a component of the relationship, which is on the mapping table.

Another way to do this is to externally inject setExtraFields() onto the filter, and make ManyManyList invoke that on filter.