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

getOwnerPage() fails when SiteTree ClassName is not a table i.e. the Page has no additional data columns than its ancestor #946

Open
rafaeldsousa opened this issue Nov 19, 2021 · 8 comments

Comments

@rafaeldsousa
Copy link

rafaeldsousa commented Nov 19, 2021

  • This commit introduces a new filter {$baseTable}.ClassName when getting the OwnerPage of an ElementalArea
  • We have CustomListPage (with no $db fields) that extends GenericContentPage. So page records are recorded on GenericContentPage table
  • But on SiteTree the ClassName of the page is saved as CustomListPage not GenericContentPage
  • With the above comment in mind, when looking for the OwnerPage and with the new ClassName filter, the page isn't found, since the ClassName coming to Elemental is GenericContentPage but SiteTree has the page class name as CustomListPage

The fix assumes that "\"{$baseTable}\".\"ClassName\" = ?" => $class matches the class, but this is not the case for classes that don't have additional db columns (e.g. classes using their parent class table as pointed in the above example). In these cases the class recorded on the SiteTree differs from the base data table.

@rafaeldsousa rafaeldsousa changed the title getOwnerPage() fails when... SiteTree ClassName is not a table / Page has no additional data columns than it's ancestor getOwnerPage() fails when SiteTree ClassName is not a table i.e. the Page has no additional data columns than its ancestor Nov 19, 2021
@rafaeldsousa
Copy link
Author

rafaeldsousa commented Nov 19, 2021

@michalkleiner I forgot to mention that the above results in the elemental areas of the affected pages not to appear on the live version, when the page is in Draft mode. That's the biggest problem we're facing with this.

From the example above

  • Edit the page/elements on page CustomListPage and just save (leaving it in Draft mode - Stage version)
  • Latest published version of elements (in elemental area) are expected to appear in FE
  • Actual result is that no elements from elemental area are displayed in FE (until page is published once again)

Our workaround in the mean time was to create a Placeholder $db field for CustomListPage (this way forcing the creation of the appropriate tables for CustomListPage) and create a build task to populate the new CustomListPage related tables.

@kinglozzer
Copy link
Member

@rafaeldsousa What’s the value of the OwnerClassName field? The $class variable in the filter "\"{$baseTable}\".\"ClassName\" = ?" => $class comes from that field.

If you’re editing a CustomListPage, as far as I can tell it should be set to CustomListPage but in your example it looks like it’s set to GenericContentPage instead. Is the problem possibly that we’re storing the wrong value for OwnerClassName?

@rafaeldsousa
Copy link
Author

rafaeldsousa commented Nov 22, 2021

@kinglozzer $class is being picked up as GenericContentPage. At some point, which I haven't debugged that far, there's an assumption that the page class is always the same as of it's owner (or table where it's saved). In the example above, CustomListPage has no $db fields and extends GenericContentPage, so no table is created for CustomListPage and all page records are saved on GenericContentPage, but on SiteTree the same page record has the ClassName as CustomListPage, the filter being done in the commit mentioned above also assumes that the className from baseTable (SiteTree in this case) is the same as the class/table name of the owner.

  • Y has $db fields
  • X extends Y
  • X has no $db fields
  • X records saved on Y table
  • SiteTree X records ClassName saved as X
  • X OwnerClassName on ElementalArea.php returns Y
  • $baseTable in new filter is SiteTree
  • $class is Y
  • No records are found due to ClassName and AreaID not finding any matches

@michalkleiner
Copy link
Contributor

@rafaeldsousa the commit introducing the check might be a red herring here.
Can you check what do you have in the OwnerClassName column in the ElementalArea table for the page instance of the page type without extra db fields? It's possible the mechanism is not storing the right class to the db in the first place.

@rafaeldsousa
Copy link
Author

rafaeldsousa commented Nov 22, 2021

@michalkleiner it seems like it's just as you've said, the className saved on ElementalArea is not the correct one.
Screen Shot 2021-11-23 at 12 29 42 PM

Worth noting that after I've applied the workaround, I've mentioned above (placeholder db field on the pagetype that didn't have fields and a build task to publish pages) the OwnerClassName on ElementalArea got updated to CustomListPage

@kinglozzer
Copy link
Member

So it sounds like we need to debug why OwnerClassName is set to the nearest ancestor with its own table instead of being set to the correct “instance” class name?

@michalkleiner
Copy link
Contributor

michalkleiner commented Nov 23, 2021

The problem relates to ElementalArea->supportedPageTypes which only considers classes backed by db tables. It iterates over the list of such classes and tries to find all elemental area relations. When it finds one that corresponds to the current page, it assigns the current class into the OwnerClassName field. But because the unbacked class is never in the initial list, it will never be assigned correctly.

We can try changing ClassInfo::getValidSubClasses(DataObject::class) in the supportedPageTypes method to ClassInfo::getValidSubClasses(DataObject::class, true), to include unbacked classes as well, and see what happens.

@rafaeldsousa
Copy link
Author

rafaeldsousa commented Nov 23, 2021

@michalkleiner Yep that's where I thought it would be coming from, I was looking for where OwnerClassName gets set and this might be the place.
But looking at the getValidSubClasses even if we add the true flag to unbacked classes, this is what happens

if ($includeUnbacked) {
    $table = DataObject::getSchema()->tableName($class);
    $classes = DB::get_schema()->enumValuesForField($table, 'ClassName');
} else {
    $classes = static::subclassesFor($class);
}

Looks like it still relies on DB table name and the CustomListPage won't have a table in this example. Wouldn't that fail still? Haven't had the chance actually test this on the site yet.

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

No branches or pull requests

4 participants