RFC: Restructure data models to use more default relationship handling

[robbieaverill]

I've had a quick look through the data model relationships in this module.

ElementalArea defines OwnerClassName and handles its value internally. This should really be at polymorphic has one relationship.

This is presumably so that things other than pages can have Elemental applied to them, for example if a dev was using DataObjects as pages and not using the CMS.

The owner (page etc) should also have the ability to define extra has_one ElementalAreas to add new areas.

The way I see it is:

- Page (or other)
  - has_one "ElementalArea" (default)
    - has_one "Owner" (inverse relationship - polymorphic)
    - has_many "Elements"

- BaseElement
  - has_one "Parent" ElementalArea

The API should support us accessing:

• the page from the element (via the elemental area or parent elements) e.g. $page = $element->getPage() (pre-existing API)
• the page from the elemental area e.g. $page = $elementalArea->getPage() (currently is called getOwnerPage, polymorphic relationship would make it Owner())
• the elemental area from the page e.g. $page->ElementalArea()
• the elements fro the page (via elemental area) e.g. $elements = $page->ElementalArea()->Elements()

Also the terminology - in some places we refer to "Owner" and others we refer to "Page". These should be standardised to "Owner" (wary of potential Extension conflicts with extending this code though). Another option Container().

I guess my RFC is to remove the custom Owner handling in ElementalArea and make it a polymorphic has one. This should be reasonably backwards compatible, but we also have the opportunity right now to make breaking changes in master for elemental 4.x.

[robbieaverill]

Basically, there is a fair amount of technical debt and responsibility in methods such as ElementalArea::getOwnerPage() that could be alleviated by using out of the box ORM relationships in this instance

[ScopeyNZ]

Huge 👍 from me. I think this is a great time to essentially fix this.

[NightJar]

More core; more default; more betterer 👍

[chillu]

Yep, we should use core features where available. The owner terminology is also used for DataExtension, so if you're using an extension on a block, you'll end up with $this->getOwner()->getOwner(), which is a bit weird. But that can be make clearer through $block = $this->getOwner(); $page = $block->getOwner(); so I'm not too concerned.

Does this have performance implications? If so, I'd consider this impact/high in terms of prioritisation.

[robbieaverill]

The performance implications should've been resolved in #368

[ScopeyNZ]

This should be adjusted to ensuring the relationship is versioned correctly. Currently publishing a page does not remember the version of an elemental area (and the blocks within the area).

Perhaps an A/C; When browsing through the history of a page with ElementalPageExtension I should see the blocks as they were at the time that page version was published.

There's some ambiguity about whether this has ever worked correctly in the past - but from my limited testing it's not working now.

This would make the issue a bit higher impact imo. It seems like having a intuitive history (especially with in-line editing) would be a pretty important feature.

[chillu]

Currently publishing a page does not remember the version of an elemental area (and the blocks within the area)

This is discussed at https://github.com/silverstripe/silverstripe-versioned/issues/195, let's split this out from this data modeling discussion if possible.

@robbieaverill The resolution to the performance aspects was described as "fixed in project code". Assuming that it's not acceptable for every larger scale blocks implementation to have project specific performance workarounds, is there work left on this card?

[ScopeyNZ]

I think it still makes sense to refactor the relationships for elemental - They're still not very sensical. However I think it has missed the cutoff for 4.0 and it's questionable how much of a performance difference it will make right now. Changing it to use a more standard approach would mean that updates to ORM performance would be inherited here though.

TL;DR I think this issue should remain.

[robbieaverill]

I agree with @ScopeyNZ - we should look at the architecture of the model relationships here.

@chillu re your question about the project code, we added optimistic relationship list caching in the project. The problems with elemental and performance are really framework problems, so the "eager loading" RFC would probably be the required step to fix this for everyone I guess

[ScopeyNZ]

Example of a bug that probably wouldn't exist if we did this: #671