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

Feature: Add extension capability #8

Open
patricknelson opened this issue Apr 26, 2017 · 5 comments
Open

Feature: Add extension capability #8

patricknelson opened this issue Apr 26, 2017 · 5 comments

Comments

@patricknelson
Copy link
Contributor

Hi, it looks like the functionality for this class GridFieldPage isn't super complicated and really just provides some methods that extend the useful grid-capable functionality. In my case, I have a pretty complicated class hierarchy in my site and, relatively deep in that hierarchy I have a one-off situation where a certain page type needs to be able to be displayed inside of a GridField. In my case, it doesn't make sense to affect 90% of the pages on my site in order to apply a few methods, so I got to thinking...

I propose either converting this to an extension (less likely) or better, adding an extension to this code base and then sharing functionality between classes via traits (easily accomplished with good compatibility via direct includes as SilverStripe 3.x apparently still has issues with autoloading traits 🤕).

I think I'll submit a PR with an example of this change. In the meantime, I'll have a custom version of this in my code base which is just a copy of this code applied as an extension.

patricknelson added a commit to patricknelson/silverstripe-gridfieldpages that referenced this issue Apr 26, 2017
patricknelson added a commit to patricknelson/silverstripe-gridfieldpages that referenced this issue Apr 26, 2017
micschk added a commit that referenced this issue Apr 27, 2017
NEW (#8): Ability to apply grid field capability to pages via DataExtension
@micschk
Copy link
Owner

micschk commented Apr 27, 2017

Very nice, thanks for this! I've been wanting to revisit this module for a while now. I've just merged your PR into master. Will test a bit and indeed create a 2.0 tag, maybe including some other functionality as well;
I have been using a patchy 'unofficial' version of this module which gets applied as an extension for quite a while now, originally because we couldn't use traits on some production systems (php 5.3, but they're all running 5.6 by now so a trait is probably the best solution for this).
Another, even more modified version I've been using depends on lumberjack functionality (which feels a bit like giving in, but it is supported and maintained by Silverstripe).

Only thing now is probably that I'd like to remove the GridFieldPage class altogether and just include it as an example of implementation. What do you think?

@patricknelson
Copy link
Contributor Author

patricknelson commented Apr 27, 2017

Welcome! Been using this for a few years now (edit: proof!) on a site with thousands of pages, myself. I've recently needed to incorporate yet another area with a particular page type that could have hundreds of pages in the long term, so GridField is the way to go, but given the site's existing functionality it makes more sense to apply this functionality to a "leaf node" of a class (page type) via DataExtension instead of updating a parent class that almost every page on the site is already using.

Another, even more modified version I've been using depends on lumberjack functionality (which feels a bit like giving in, but it is supported and maintained by Silverstripe).

You completely lost me there! What is this "lumberjack" you speak of?

Only thing now is probably that I'd like to remove the GridFieldPage class altogether and just include it as an example of implementation. What do you think?

i.e. do you mean to go extension only, then to recommend that if a user wishes to apply via built-in class inheritance that they take the standard SS extension (e.g. apply via config.yml configuration) and then apply that to some parent class and then extend it? That should work since in SS the extensions applied to objects which are parents of the currently instantiated DataObject also apply to children.

NOTE: I'm not sure if it's possible to disable an extension for child classes, but if it were, that'd be another advantage to going DataExtension-only.

@patricknelson
Copy link
Contributor Author

patricknelson commented Apr 27, 2017

Also, actually, I've been using Page's in GridField's for quite a while in conjunction not only with this module but also with your other module silverstripe-gridfieldsitetreebuttons. Personally I think they should be merged as I see no point to keeping them separated, as I think improvements can be integrated between the two.

If you're listing pages in a GridField, there's a very good chance you'll need to also add/edit/remove them from there as well. I've also got my own GridFieldDeletePageAction class to compensate for the fact that ->delete() doesn't really delete instances of SiteTree unless a special ->doUnPublish() method is also called.

I've also created my own GridFieldAddPageButton (to supersede your GridFieldAddNewSiteTreeItemButton) in order to incorporate SilverStripe's built-in security mechanisms associated with generating new pages. It does this by taking advantage of CMSPageAddController to actually create the page so that permissions can be properly checked first. The only disadvantage is that it is tightly bound to the URL structure /path/here/$SomeID since it just calls ->doAdd(...) and then parses the URL in the Location response, as unfortunately SS is too much of a tangled mess to abstract this functionality without requiring integrating with HTTP-level request/responses...

This code goes int' the GridFieldAddPageButton's ->handleAction() method.

// Initialize data that we'll use to pass to our controller which actually does the real heavy lifting.
$data = [
	'PageType' => $gridField->getModelClass(),
	'ParentID' => $this->getParentID(),
];

// Just so we have an instance to pass (would be required if there's a validation error). This just prevents crashing.
$form = new Form(new Controller(), 'FormStub', new FieldList(), new FieldList());

/** @var CMSPageAddController $addController */
$addController = singleton('CMSPageAddController');
$addController->doAdd($data, $form);

// Pull the newly created page ID from URL in the location header. This is better than relying on global state to be
// modified (i.e. looking instead at $editController->currentPageID() which is initialized inside of ->doAdd()).
$response = $addController->getResponse();
$headers = $response->getHeaders();
if (isset($headers['Location'])) {
    // ... magic ✨
}

@patricknelson
Copy link
Contributor Author

patricknelson commented Apr 27, 2017

Oh, and I forgot -- to tie it all together, I've also got a GridFieldConfig_VersionedRecordEditor class which creates the necessary default configuration to initialize the buttons and setup the GridField. It also has programmed-in error messages to ensure you don't attempt to add a delete button to the grid until you've confirmed that you are fine with pages being deleted completely without being archived or just unpublished first, i.e.

if ($hasDelete && !$obj->canHardDelete()) {
	user_error("Warning: Using a GridFieldConfig_VersionedRecordEditor with a versioned SiteTree object will " .
	"allow deletion of an object which will cause it to be orphaned in published mode but potentially unable to " .
	"be accessible in CMS (if it's not visible in the CMS left-hand site tree). Please be sure to enable " .
	"->hard_delete = true on these objects.", E_USER_WARNING);
}

@patricknelson
Copy link
Contributor Author

👆 Key point there is that deleting a versioned SiteTree object from a GridField could result in an orphaned and thus inaccessible instance of the page still visible in production but unable to be found in the CMS due to the base table record being removed.

Sorry for being scatterbrained, I'm busy but had a lot to say and haven't organized my thoughts 😭 Let me know what you think about maybe integrating those features into either of these repositories or potentially merging them or even both.

patricknelson added a commit to patricknelson/silverstripe-gridfieldpages that referenced this issue Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants