dynamic / silverstripe-foxy

BSD 3-Clause "New" or "Revised" License
1 stars 3 forks source link

BUG Variation - currently is hard coded to look for a has_one relation to Product #106

Open jsirish opened 4 years ago

jsirish commented 4 years ago

Describe the bug Currently there are multiple methods where a Variation is looking for a has_one Product. Throughout the rest of the Foxy modules, we've tried to keep this more flexible so that multiple classes in the same project could have ecommerce functionality - such as a Product, Event, etc - without having to extend a base Product class

Expected behavior Multiple, unrelated classes could have Foxy extensions such as Purchasable and Shippable applied to them and still work with Variations

Additional context We could explore a generic has_one Product => SiteTree::class relationship. This would limit products to being Pages, but there are already instances across Foxy modules where we've made this assumption. So assuming that would work, it would be an acceptable implementation based on the current code structure.

muskie9 commented 4 years ago

I think this would have to be more polymorphic and do 'Product' => DataObject::class similar to the example in the Silverstripe docs for Polymorphic has_one. I think with how this is currently setup, the assumption is the relation name vs the class on the actual relation. The has_one has to be implemented by the developer per project, so it could end up being any class, however if there is no common base class issues could arise.

I could be misremembering, but I'm for polymorphic relations if they work as they allow the flexibility we're looking for.

jsirish commented 4 years ago

The polymorphic works in some cases. However, the issue we run into is when we want to query by a particular field, such as Code, that does not exist on that base class, be it DataObject or SiteTree. I ran into this recently with the Inventory checks for Variations.

Somehow we've made it this far without hard coding a base class in most cases, silverstripe-foxy-products being the exception (by design).

There is this method in FoxyHelper that gets a list of the array of classes spec'd as "Products", which is used in the FoxyHelper->getProducts() method. Maybe there is an opportunity here:

https://github.com/dynamic/silverstripe-foxy/blob/1.2/src/Model/FoxyHelper.php#L163

muskie9 commented 4 years ago

I think something similar would work at this point and re-visiting how we can support the queries in a better way is worth another look. Now that you mention the field issue I recall why we have it setup this way. We could explore storing the "Owner" type class in a Varchar on a variation.

This type of setup would be similar to what ElementalArea does. We may have to review how this would then be leveraged, but that would give us the class of the related product. Depending on the relation calls, we could determine how that would or wouldn't be useful in those situations.