silverstripe / silverstripe-cms

Silverstripe CMS - this is a module for Silverstripe Framework rather than a standalone app. Use https://github.com/silverstripe/silverstripe-installer/ to set this up.
http://silverstripe.org/
BSD 3-Clause "New" or "Revised" License
511 stars 333 forks source link

refactor CMSMain::PageTypes to use SiteTree::getClassDropdown #2208

Open sunnysideup opened 6 years ago

sunnysideup commented 6 years ago

consider using SiteTree::getClassDropdown instead of SiteTree::page_type_classes in CMSMain::PageTypes as getClassDropdown and PageTypes almost does the same thing, but providing the final data from SiteTree rather than CMSMain seems better.

maxime-rainville commented 6 years ago

Could you clarify what you're suggesting here?

Are you suggesting we deprecate CMSMain::PageTypes and encourage people to use SiteTree::getClassDropdown instead?

sunnysideup commented 6 years ago

Here are the links:

CMSMain::PageTypes https://github.com/silverstripe/silverstripe-cms/blob/4/code/Controllers/CMSMain.php#L1107-L1137 Gets data from SiteTree::page_type_classes and removes:

returns SS_List of items with a bunch of entries.

Also not that there is a function getPageTypes in CMSMain: https://github.com/silverstripe/silverstripe-cms/blob/4/code/Controllers/CMSMain.php#L955-L963 That does something completely different ... I will create a separate ticket for that.

SiteTree::getClassDropdown https://github.com/silverstripe/silverstripe-cms/blob/4/code/Model/SiteTree.php#L2485-L2534 gets data from SiteTree::page_type_classes and removes:

https://github.com/silverstripe/silverstripe-cms/blob/4/code/Controllers/CMSMain.php#L955-L963

SiteTree::page_type_classes https://github.com/silverstripe/silverstripe-cms/blob/4/code/Model/SiteTree.php#L470-L506 Takes all site tree descendants (class X extends SiteTree) and removes the ones marked in hide_ancestor

What I am suggesting is that CMSMain::PageTypes uses SiteTree::getClassDropdown rather than repeating its code and that we add a param to SiteTree::getClassDropdown where the context can be set (with the ability to set it to null). Or something like this. The reason I am suggesting this is so that we have one source for the list of page types.