History does not work for classes extending SiteTree

[mikenz]

History makes the assumption that all pages on the site are classes that extend Page (https://github.com/silverstripe/silverstripe-cms/blob/4/client/src/state/history/readOnePageQuery.js#L10).

This is an incorrect assumption and when attempting viewing the history of an item that extends SiteTree, but not Page, the user just receives a spinning SS. Any class that extends SiteTree is a valid page, and history should work correctly for these.

Unfortunately this impacts over 50% of our pages.

[chillu]

@unclecheese This is a tricky one. One the one hand, queries like readOnePage are designed as a basis for a "Content API" provided by the CMS - and readOneSiteTree is just a shitty way to name this for API consumers. But on the other hand, Mike is correct that we can't assume everything extends Page.

Option A: Refactor to readOneSiteTree, remove readOnePage (it's not part of the public API)
Option B: Refactor to readOneSiteTree, retain readOnePage as well (will slow down schema generation)
Option C: Retain readOnePage, use Sitetree::get(), leave missing Page fields as nulls (not sure how easy this is with both GraphQL v3 and v4)

[sminnee]

I would note that using pages that don't extend Page is an edge-case behaviour and so I'd design the API to treat it as such.

That would probably push us toward Option C. A couple of downsides:

• it ends up being a bit of inconsistent magic dust in the API design which may get confusing down the road (not a showstopper but worth mentioning)
• by default, SiteTree::get() won't join to the Page table (those fields are lazy-loaded) but Page::get() would. This may change query performance in the general case (history itself might be okay) but we could probably tweak the query generation to help with this, after eager loading has been pulled into core (because it would touch code heavily refactored in that branch)

Given the above Option B might be easier... how much slower are we talking?

[unclecheese]

Trivially slower. I think, very generally speaking, test show that 50 dataobjects with relationships and versioning, fully loaded, every feature enabled, no caching, is about 7 seconds of generation. One little page class with read only exposed is easily less than 300ms.

Wondering, is this just a case of better documenting how important Page is to the CMS, and that breaking this convention has consequences? In SS4, I can't see why you would ever want to do this short of a lack of understanding.

[chillu]

@mikenz What was your rationale for skipping the Page class? It's a pretty lightweight class after all. This might also be a trigger to state that it is indeed a requirement to use extends Page when working in the CMS.

[kinglozzer]

@chillu We do this too, mainly because when originally starting out with SS4 I wanted to ensure as much as possible was PSR-4 compliant. The un-namespaced Page doesn’t really fit into that very nicely, so at the time I opted to work around it like this:

https://github.com/bigfork/silverstripe-recipe/blob/master/app/src/bootstrap.php, loaded with composer.json

So adding an example class of HomePage, the hierarchy would be:

• SilverStripe\CMS\Model\SiteTree
  • App\Model\Page -> this class is hidden from CMS users with hide_ancestors
    • Page -> This works with readOnePage
    • App\Model\HomePage -> This doesn’t work with readOnePage as it doesn’t extend Page

I remember wondering at the time if it might come back to bite me 😉

[chillu]

We've had a related discussion back in 2016 about namespacing Page. The accepted solution there was the same as Loz' example from above.

@silverstripe/core-team I'm recommending that we document the requirement to use extends Page as a baseline in the CMS, and explicitly state that we don't support extends SiteTree. In my view, that's just documenting the accepted status quo, but it also makes a clearer statement about the validity of edge cases like the one Mike brought up here.

[michalkleiner]

Should Page and SiteTree merge back together from SS5?

[kinglozzer]

We've had a related discussion back in 2016 about namespacing Page. The accepted solution there was the same as Loz' example from above.

@silverstripe/core-team I'm recommending that we document the requirement to use extends Page as a baseline in the CMS, and explicitly state that we don't support extends SiteTree. In my view, that's just documenting the accepted status quo, but it also makes a clearer statement about the validity of edge cases like the one Mike brought up here.

I was wrong in my comment in that thread - the accepted solution there doesn’t work. Page itself works, but any other pagetypes in project code don’t because they extend App\Model\Page, not \Page