singleton($step['class'])->extend('augmentSQL', $sql); in SearchIndex::getDirtyIDs() causes throwing Fatal Errors

[normann]

singleton($step['class'])->extend('augmentSQL', $sql); in SearchIndex::getDirtyIDs() function breaks some DataExtension objects when calling to their function augmentSQL(), eg. Translatable, or SiteTreeSubsites, since Translatable::augumentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) is expecting the second parameter is a DataQuery object and directly uses it to call its function getQueryParam() in its implementation without checking if $dataQuery is a non-object or not

So this issue could be either fixed in Translatable class or SearchIndex class. Giving that if we fix here in SearchIndex, non of those DataExtension classes need to do anything, I hope it could fixed here.

[tractorcow]

I think it does need to be fixed in fulltextsearch. That parameter is required in some cases.

[dhensby]

If Translatable's signature is $dataQuery = null then it really shouldn't blindly expect an object...

@tractorcow how should we obtain a valid DataQuery in SearchIndex::getDirtyIDs() seeing as it doesn't seem to be using the ORM at all...

[tractorcow]

It's more = null for legacy reasons, that is we couldn't change the method signature. We should have in 4.0 though to make it mandatory.

It should always provide it though; Versioned relies on it for example.

hm

$sql = new SQLSelect('"ID"', '"'.$tableName.'"', '"'.$step['foreignkey'].'" IN ('.implode(',', $ids).')');
singleton($step['class'])->extend('augmentSQL', $sql);
$ids = $sql->execute()->column();

Should probably be:

$ids = DataObject::get($step['class'])
  ->filter($step['foreignkey'], $ids)
  ->column('ID');

Not sure why it's written in such a convoluted way.

Similarly for the other relations... stop trying to do raw-sql, and just use the ORM the way it's meant to be called?

[tractorcow]

Had a guess at how the code would look all up:

foreach ($derivation['chain'] as $step) {
    if ($step['through'] == 'has_one') {
        $ids = DataObject::get($step['class'])
            ->filter($step['foreignkey'], $ids)
            ->column('ID');
    } elseif ($step['through'] == 'has_many') {
        // Get many ids by querying component with alternate set of foreign ids
        $ids = DataObject::singleton($step['otherclass'])
            ->getComponents($step['method'], $ids)
            ->column('ID');
    }

    if (empty($ids)) {
        break;
    }
}

This makes use of the fact that you can mock a HasManyList with a set of foreign $id keys (instead of a single parent id).

Good thing I added that second arg to getComponents() :)

[michalkleiner]

Affecting MBIE on a CWP project using 2.1 recipe

[UndefinedOffset]

This also seems to affect Versioned objects as well in at least SilverStripe 4.3.0, Versioned requires a DataQuery as the second argument even though it's defined as accepting null as a parameter (see here).

[michalkleiner]

The proposed PR works for us on the project, just need to sort the tests. Will try looking at it ASAP.

[UndefinedOffset]

I think all you'll need to do is update that one line referenced in #239 to fix it there, the same fix is needed in #240 but you also need to replace DataObject::singleton() as the singleton method is not in SilverStripe 3.x :). Hope that helps :)

[michalkleiner]

Even with the updates the test getting the list of dirty IDs for has_many is failing. Will try to look into it more.

[michalkleiner]

Back on this again, only 4 years after the initial report.

[tractorcow]

@michalkleiner I haven't used SOLR for a few years, but revisiting, I think the code example I pasted above should still fix the issue. I suggest throwing it into a PR and testing it out on a few live SOLR indexes to see if it works.

[michalkleiner]

@tractorcow it works for us on a project, yes. So you reckon it's rather about the failing test that might need adjusting?

[michalkleiner]

Phew, ok, two new PRs with the tests passing and using the simplified DataObject::get approach.