-
Notifications
You must be signed in to change notification settings - Fork 821
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
Add onBeforeUnlink and onAfterUnlink method support to the GridFieldDeleteAction Component #11366
Comments
Proposed Code for GridFieldDeleteAction.php (extra spacing added for clarity): public function handleAction(GridField $gridField, $actionName, $arguments, $data)
{
//...
if ($actionName == 'deleterecord' || $actionName == 'unlinkrelation') {
//...
if ($actionName == 'deleterecord') {
//...
} else {
//...
// NEW onBeforeUnlink code
$hookSupport = $item->hasMethod("invokeWithExtensions");
if ($hookSupport) {
$item->invokeWithExtensions("onBeforeUnlink", $gridField, $this);
}
$list->remove($item); // present in codebase
// NEW onAfterUnlink code
if ($hookSupport) {
$item->invokeWithExtensions("onAfterUnlink", $gridField, $this);
}
}
}
} I am not sure what parameters would be most useful, so I just added the Gridfield and the component itself. But let me know if other parameters should be used instead |
I wouldn't tie this to I've seen similar requests for adding hooks for when adding records to a relation list too, so it's starting to feel like something along these lines is necessary. The main problem with that would be that adding/removing records to relation lists is a very high traffic piece of code, so before I merge anything new extension hooks in that area I'd want to see some profiling that shows it won't cause performance issues at scale. |
I was thinking the same. Perhaps better suited for |
Oh. Well. That makes sense. And I see your point, it would be more useful at a lower level. But alas I don't have any ideas for how that might be done. I don't suppose it could be a progressive enhancement that's transparent to the end user? Like implementing in the component first, then later removing it and replacing it with the lower level version, but keeping the API the same? |
It could, but I don't see the value in adding an extension hook we intend to remove. If we're going to do this, IMO it should be done at the lower level from the start. |
You could try Injector override, but I don't think the components are created via Injector to override the GridField Delete button component. Best workaround for now would be to implement your change where you want it, then use a combination of removeByType and addComponent with your GridFieldConfig (or via an Extension if a dependency) before using it. |
Description
I propose that we use
invokeWithExtensions()
to addonBeforeUnlink()
andonAfterUnlink()
support to objects that are going to be unlinked using the GridFieldDeleteActionIf this is added to the core, it will mean that all DataObjects (or any other Gridfield managed object) will be able to optionally define these methods to run code before or after that object has been unlinked
Additional context or points of discussion
A few extra thoughts on the matter:
This is something that I have found useful in my own development. In my case I only used it in a few places, so I just subclassed the GridFieldDeleteAction and used my subclass in Gridfields that I needed to support the feature. But I can see this as being a simple (I think?) but useful addition to the core.
Support for non-Extensible objects - With recent updates it is safe to say that going forward not all Gridfields will manage DataObjects and so its plausible that some of the objects may not support the
invokeWithExtensions()
method that is apart of the Extensible trait. I propose that we add a conditional check around the hooks to ensure that either the method exists, or that the class uses the Extensible trait (perhaps with class_uses)?Documentation - I would also add some documentation relating to this new feature. I'm thinking adding it to the Extending DataObject models (as invokeWithExtensions() allows for extension support), and Introduction to the data model and ORM pages would be best. But what do you all think?
Validations
The text was updated successfully, but these errors were encountered: