Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Don't assume a DataObject's table. #204

Conversation

NightJar
Copy link
Contributor

@NightJar NightJar commented Feb 23, 2018

Knowing if a DataObject has a table or not is irrelevant - data is queried
out and pushed into solr all the same, whether there is a join table, a
base table, or any combination with maybe a few no-tables in between.
SomeDataClass::get() will fetch all data irrelevant of tables, that's the
point of an abstraction like SilverStripe's ORM.

The index featured is a default install, where Page is defined as

use SilverStripe\CMS\Model\SiteTree;

class Page extends SiteTree
{

}

and defines an index adding only Page::class to it.

capture

Resolves #168

Knowing if a DataObject has a table or not is irrelevant - data is queried
out and pushed into solr all the same, whether there is a join table, a
base table, or any combination with maybe a few no-tables in between.
SomeDataClass::get() will fetch all data irrelevant of tables, that's the
point of an abstraction like SilverStripe's ORM.
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the reasoning but would like to test it before merging

@NightJar
Copy link
Contributor Author

Seems fair :)

@dhensby
Copy link
Contributor

dhensby commented Feb 23, 2018

wow - what is this about? what a bizarre constraint. I don't see that this could have any regression for existing code at all - this simply allows us to add all DO classes instead of an arbitrary subset of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants