silverstripe/sapphire — pull request #716 by tractorcow (2012-08-10 02:00:21+00:00)
Title: FIXED: Crashed caused by viewing versioned page

When viewing the archived version of a page the augmentSQL function of the Versioned extension would generate invalid SQL. E.g.

```sql
SELECT
...
FROM SiteTree_versions
AND Page_versions.Version = SiteTree_versions.Version
INNER JOIN _ArchiveSiteTree ON _ArchiveSiteTree.ID = SiteTree_versions.RecordID AND _ArchiveSiteTree.Version = SiteTree_versions.Version
...
```

This is due to an odd $query->addFrom which attempts to re-add an existing from table name, but with an additional join condition; However, it doesn't actually do that, but it adds a SQL fragment "AND .... " in the place of the table name. It's possible that I may have misunderstood the intent of this code. Possibly it assumes the addFrom will perform a recursive table/join/condition merge. Actually, addFrom instead completely replaces the existing from condition.

The actual addFrom is not necessary as the target table would already be listed in the from items.

I have replaced the addFrom with an addWhere condition to add in the new join condition. This however doesn't add the condition in the actual join itself, but in the 'where' clause. This is still valid SQL and achieves the same purpose. I tried a version of this code using addFilterToJoin, but not every "from" in a query (such as _ArchiveSiteTree) would be guaranteed to be in an array format (that addFilterToJoin requires). Could add additional code to check, ensure queries in required format, joined classes are subclasses of $baseTable, but, code is a burden, so simpler is better.

This has been tested against the VersionedTest and passes all 12 tests.

Might need to test this against other modules which augmentSQL (subsites, translatable, etc).

Additionally, there was as small but relevant bugfix where versioned tables were being joined against the wrong fields (ID,Version instead of RecordID, Version). This came about because in the $query->replaceText the tables were renamed first from table to table_versions, and then the string replacer replaced table.id with table.recordID (which by this time, no longer existed).

So much testing investigation and documentation for a small change. :)