Skip to main content

This site requires you to update your browser. Your browsing experience maybe affected by not having the most up to date version.

Form Questions /

Possible patch for field collisions


Reply


6 Posts   783 Views

Avatar
nykayfabe

Community Member, 4 Posts

4 June 2010 at 3:10pm

Edited: 04/06/2010 3:40pm

Please forgive me if this is all wrong. I'm working from an older version of SS (2.3.1) which was heavily customized and hopelessly out of sync, making an upgrade a major hassle. Anyway, when a page type is changed and a field of the same name exists in both the old and new type, sometimes the field value is not retrieved correctly (the value from the old page type record comes through). I think the fix I came up with would work even with the most current build but it's so simple that I'm doubting it. In sapphire/core/model/DataObject.php > buildSQL change:

$query->from[$tableClass] = "LEFT JOIN \"$tableClass\" ON \"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"";

to:

$query->from[$tableClass] = "LEFT JOIN `$tableClass` ON `$tableClass`.ID = `$baseClass`.ID AND '$tableClass' = `$baseClass`.ClassName";

The latest version of DataObject attempts to fix this by using the array, collidingFields, and then adding a WHEN clause to the query. I actually attempted to flesh this out before I came up with the above and it resulted in some humdongous queries which crashed our live site.

That said, does anyone see anything wrong with the fix I've posted? FWIW, it seems to be working fine on my test machine in that all pages are rendered correctly and the fields which were previously getting the wrong value in the CMS are now correct. (Incidentally, the collision problem can also be resolved by manually deleting the record from the old page type table, but that's a pain to do on the regular.) Thanks in advance.

Avatar
Willr

Forum Moderator, 5513 Posts

4 June 2010 at 3:51pm

That said, does anyone see anything wrong with the fix I've posted?

Well the first issue is that your patch isn't using the correct format for escaping queries so it wouldn't be considered for merging back. See http://doc.silverstripe.org/coding-conventions#writing_queries_for_database_abstraction. As for the functionality of the patch I'm not qualified to know it'll break anything - have you run the unit tests after making that change?.

Avatar
ajshort

Community Member, 244 Posts

4 June 2010 at 6:42pm

Edited: 04/06/2010 6:42pm

OK, from what I understand, if you have a tree of classes that runs SiteTree -> Page -> BlogEntry, and you perform a DataObject::get_one('SiteTree'), the following joins will be created:

+ SiteTree
   + Page -> LEFT JOIN "Page" ON "Page"."ID" = "SiteTree"."ID" AND "SiteTree"."ClassName" = 'Page'
      + BlogPage -> LEFT JOIN "BlogPage" ON "BlogPage"."ID" = "SiteTree"."ID" AND "SiteTree"."ClassName" = 'BlogPage'

If you have a record with ClassName "BlogEntry" across all those tables, you would expect all the data from all the tables to be returned in that record. However, since "SiteTree"."ClassName" == 'BlogPage', the join condition for the Page class will return false (since it is comparing it wirh 'Page') - so any data from the Page table won't be returned.

I could be wrong, but I think this patch will break things.

Avatar
nykayfabe

Community Member, 4 Posts

5 June 2010 at 3:04am

Thanks for your reply aj. I think I understand what you're saying but I don't believe it's a problem. I ran DataObject::get_one('SiteTree') in the controller from a page with the same hierarchy as your example, SiteTree -> Page -> IndividualEntry, and it produces a query that includes this:

LEFT JOIN `Article` ON `Article`.ID = `SiteTree`.ID AND 'Article' = `SiteTree`.ClassName
LEFT JOIN `CareerOpportunities` ON `CareerOpportunities`.ID = `SiteTree`.ID AND 'CareerOpportunities' = `SiteTree`.ClassName

...so on and so forth. The query, in fact, does return every record in SiteTree because the second AND clause in the JOIN is always satisfied because there is no WHERE clause to limit the ClassName. In the CMS, however, the queries have a WHERE clause (e.g.: "WHERE `SiteTree`.`ID` = 127") which means that the SiteTree.ClassName being evaluated is the one from the record with that specific ID, the ID of the page you're editing.

A different query fired from the same page's controller, DataObject::get_one('Article') correctly only returns all records in the Article table:
SELECT `SiteTree`.*, `Article`.*, `SiteTree`.ID, if(`SiteTree`.ClassName,`SiteTree`.ClassName,'SiteTree') AS RecordClassName FROM `SiteTree` LEFT JOIN `Article` ON `Article`.ID = `SiteTree`.ID AND 'Article' = `SiteTree`.ClassName WHERE (`SiteTree`.ClassName IN ('Article')) ORDER BY Sort

If I've misunderstood your concerns or misinterpreted your example, please let me know. Thanks again.

Avatar
ajshort

Community Member, 244 Posts

5 June 2010 at 3:26am

I just tried your patch on my local site and it broke a heap of stuff. If i have the page structure SiteTree -> Page -> CustomPage, where both Page and CustomPage have custom db fields, and try something like:

var_dump(DataObject::get_by_id('Page', 1)->Test);

it will return null, whereas when i revert the patch it works fine.

Avatar
nykayfabe

Community Member, 4 Posts

5 June 2010 at 3:35am

I guess it only works with our hacked up version of 2.3.1 because if I do the following, it correctly prints the title of the first page in SiteTree:
var_dump(DataObject::get_by_id('Page', 1)->Title);

Weird, wild stuff. Thanks for your help and I'm sorry that my "solution" isn't universally applicable.