NEW: Add/remove callbacks for relation lists #9572
Conversation
|
@sminnee I know @unclecheese is looking into “relation changed” hooks as part of some work on silverstripe-search-service: silverstripe/silverstripe-search-service#29 (comment). Do you think the same functionality you’re aiming for here could be achieved with those hooks + an extension? |
At a minimum the two pieces of work should be considered together. My use-case is more "creating custom relations" than it is "responding to changes in relations", and while it's possible that the same extension points could be used for both, it's possible that they're not. Notably, I very much don't want this use case to be driven by injecting event handlers system, wide, whereas I suspect that Aaron. does. |
|
I've marked this as a draft until I talk to @unclecheese |
|
Aaron's proposed DataObject::onRelationChange, which would hook into all the relations of a given dataobject without patching the relation methods. This proposes a callback that you can apply to a specific RelationList object. I think it's different, but perhaps a unified
Any thoughts @unclecheese? |
|
With extensibility enhancements like this, I don't think we should worry about having too many options. I think the cases here have a bit of overlap, but not so much that I'm opposed to having both. Sam, do you think we need to have callbacks for |
|
I believe that setByIDList just calls add/remove internally, so I don't think it necessary and will risk creating bugs that appear only when you use setByIDList but not add. Adding a remove callback would be consistent though. |
|
A potential approach is that the feature in this PR is used to implement DataObject::onRelationChange(): DataObject, could insert a callback that calls the appropriate onRelationChange handler. This would meant that RelationList doesn't need to know about the semantics of DataObject::onRelationChange. |
I think we sometimes get pretty burned by our exhaustive extension API, and whether that falls into "public API" from a SemVer perspective. If Sam's proposed API is more flexible, let's rely on that and use it in the search service rather than implementing two pieces of code that aim to achieve the same goal. |
|
I don’t think the API I’ve proposed is a good fit for Aaron’s use-case, as he wants to be able to plug into any standard dataobject/relation. My suggestion was that the two APIs could be layered, however. It would be possible (but clumsier) for me to achieve what needed to on this with DataObject::onRelationChange. My view is that both APIs have their use-case but I’m mindful of the risk of bloating the API surface. If we were to go with only one of these, it would probably be DataObject::onRelationChange. However the best bet will be to address both of them in a single PR. |
|
Sure. I trust you to make a good judgment for what's required here considering I didn't read too much into it. I was just a little scared about the idea of "too many options" meaning two chunks of API that serve the same (approximate) purpose. |
|
I've followed up with a removeCallback in #9588. Both these implementations have an issue with list identity: It's not stable between calls. Given these lists can modify their return values through |
Hmm, maybe we can cache them, they were designed as value objects (returning clones on modification). But I think this isn't a blocker for merging these two PRs, it just limits their usefulness (and adds a few more sharp edges) |
Adding caching, presumably to getManyManyComponents, sounds like a can of worms unrelated to this change IMO. I'm not saying that it's a bad idea, but that is should be considered as more of a general thing and not in relation to making this change more useful. This use case relevant in 0.1% of the situations that a caching change would impactt. |
|
Talked with Sam, and we decided to fold my follow up PR into this one here, since they're really two sides of the same coin, and should be discussed+merged together: #9588 |
|
OK this is ready for merge from my perspective. Since it's a composite PR from Sam and myself now, maybe @kinglozzer @ScopeyNZ or @dhensby could look at this? |
|
One thing about this work: should it be pushed up to DataList instead? A DataList with an addCallback and removeCallback applied could be used to build a "virtual relationship". For example: // data object Item has a Status Active/Inactive field
function OpenItems() {
$items = Item::get()->filter(['Status' => 'Active']);
$items->setAddCallback(function ($relation, $item, $extra) {
$item->Status = "Active";
});
$items->setRemoveCallback(function ($relation, $item, $extra) {
$item->Status = "Inactive";
});
}It would be mean that other data lists could be placed into GridFields (e.g. the top GridField of a ModelAdmin). Right now:
So for this to work, DataList would also need a mechanism for disabling the default
|
|
I've tried to implement this but can't make it work consistently:
|
|
OK that's good feedback, Ingo. Let's hold off merging this right now until we've given it a bit more thought. A few questions:
I'm okay in principle with accepting that pushing the work to DataList is too hard, but if we do that we're unlikely to be able to loop back around and add it to DataList later. So we decide, one way or the other, now. |
|
I've sketched this out in a new commit: 7684da5. Check the
It'd make the relationship between the callbacks and delete-on-remove clearer for the
I'd prefer that to creating a weaker "composite API" with three args.
Yep, I'd rather switch Buuuut, that's also not possible with the current API, since you have Another use case for |
You could avoid a 2nd argument by having a private property that is set/unset to modify the built-in behaviour. Nasty, but does the job? I dunno... As an aside: This is exactly the kind of minor-detail-but-still-technically-an-API-change that I think a major release could allow. Especially if our code upgrading stuff was smart enough to highlight removeByID overrides in your module/project code. |
It would have to be protected, since In conclusion, I've implemented Sam's suggestion to extend the scope to |
|
@chillu I've found back your change to extend this to DataList (I have it on a temp branch locally in case we need it back). I think it's too hard and we should focus only on the RelationList changes as originally designed. |
Yeah... fair point. |
sminnee
changed the title
|
Callback stacks provide flexibility, and complicate APIs. We'd need an If we added getters for the single callbacks, you could wrap those callables in your own callable, which would satisfy the use case Robbie outlines. I don't feel strongly about this, but it makes me sad that this lead to such an API surface explosion. |
|
Yeah, Ingo's comment about complexity is valid; my only hesitation with merging a simple API is that adding the complexity later would be harder with BC. If we did want to allow chainable callbacks I would probably introduce a simple // Rest the list to a single closure
$list->addCallbacks()->set($closure);
// Add one on the end of the list
$list->addCallbacks()->add($closure);
// Clear the list
$list->addCallbacks()->clear();
// Get an array of callbacks
$list->addCallbacks()->getAll();
// Add one with a string identifier
$list->addCallbacks()->add($closure, 'samsOne');
// Get one by an identifier
$list->addCallbacks()->get('samsOne');For this specific use-case, adding CallbackList would likely be overkill, but are there other places where we make use of a list of callbacks? |
|
Can you use an ArrayList instead of adding a CallbackList? |
|
It's got quite a lot of other stuff and is primarily design to manage arrays of data for template population rather than callbacks. It also lacks the ability to do removal / lookup options by named identifiers, which is more of a nice to have. Also prevents me from providing a method on the class that calls all the callbacks. So I don't the trade-off for reuse there quite stacks up. The piece of ArrayList that you would benefit from is basically: class Minimal {
public $calls = [];
function push($item) { $this->calls[] = $item; }
}If we did make a CallbackList, not sure where it would belong. I'm tempted to release it as a micropackage :-P |
|
Only half joking :-P https://github.com/sminnee/callbacklist But yeah if we prefer these can be pulled into somewhere in framework. |
|
Hah, of course you've written a micropackage for it ;) I like the approach of externalising the management into it's own object, but then we should adapt that as a pattern. Done a quick survey in core, not a lot of precent at least on the surface: Before writing my answer, I also looked into https://symfony.com/doc/current/components/event_dispatcher.html, which @unclecheese has been using for <?php
class MyObject extends DataObject
{
private static $many_many = ['Things' => 'Thing'];
public function getThings()
{
$list = $this->manyManyComponent('Things')
$list->getDispatcher()->addListener('RelationList.add', function (Event $event) {
// ...
});
return $list;
}
} |
|
Yeah the event dispatcher is another way of looking at this, and it could be useful as long as we keep the following separate:
The latter case is what this PR covers. The former case is what is typically thought of in "event system" discussions. |
|
I was wondering if this can be solved by scoping the event on a more global dispatcher, e.g. fire both |
|
Yeah having looked into this a little more I think the event dispatchers and what this patch does are too different to be unified into the same subsystem. As I understand it you would need to do the following to replicate the example in the original ticket, using event-emitters. MyClass.php function MyRelation() {
$rel = $this->getManyManyComponents(‘MyRelation’);
$rel = $rel->filter(‘Status’, ‘Active’);
// Something like this necessary to hook into a new event type
$rel->setRelationName('MyRelation');
return $rel;
}MyRelationStatusUpdater.php class MyRelationStatusUpdater implements EventHandlerInterface {
public function fire(EventContextInterface $context): void
{
$id = $context->get('id');
$item = MyClass::get()->byID($id);
$item->Status = ‘Active’;
$item->write();
}
}event-config.yml SilverStripe\Core\Injector\Injector:
SilverStripe\EventDispatcher\Dispatch\Dispatcher:
properties:
handlers:
# Arbitrary key. Allows other config to override.
orders:
on: [ MyRelation.relationAdd ]
handler: %$MyRelationStatusUpdaterYUCK! This is a good approach for tidying loosely coupled parts of a system together, but not for writing a single piece of cohesive code as outlined in the callback-based example. The core issue is that the dispatcher is a singleton, from everything I read in Aaron's and the Symphony docs. |
|
@unclecheese is an instance-level dispatcher beyond the scope of what the event system is really designed for? My initial read is that the event system isn't fit-for-purpose for this use case (which is fine, it can't be everything to everyone) but I'd like to check that it's not a misunderstanding on my part before boxing on with this PR. |
|
You're right Sam, my prior example with an inlined Overall, your code example looks very similar to what you'd need for an I think we've explored the space enough to vote. Option A: Keep single setCallback(), deal with edge cases through callback nesting - original PR My vote goes for Option C, and picking up the EventDispatcher discussion at a later point without blocking this PR. |
|
Yeah I'd go with C too. Callbacks for cohesive code, DI and events for loosely coupled code. They're two different things. |
|
My only question on option C is - leave it as a micropackage or inline the class? I think a micropackage works because is got a very clear boundary of responsibility. It's one more composer download but shouldn't change much so will be cached in most setups. I'd probably tag it as 0.1.0 for the purposes of the PR. It's under my name so responsibility for the maintenance of the package would fall to me in the first instance. But happy either way. |
|
Yep happy with a micropackage. It'll become part of our API surface (through RelationList->addCallbacks()` etc). But that seems kind of unavoidable, even if we created a class interface for it, since the micropackage would need to implement that interface, and you'd end up with a two-way dependency. Given Sam as the maintainer, I'm not too worried about the micropackage status, and a cursory search on packagist also didn't turn up anything similar. |
|
Cool, I'll sign up for that. If @silverstripe/core-team have concerns with this approach, let me know. :) |
chillu
changed the title
|
@sminnee OK I've refactored this to use your micropackage, and added a few more tests. Could you release a I've also noticed that the callbacks inherit some API messiness from the list APIs: The |
My preference would be that we 0.1.0. I feel like "Ideally we'd have downstream packages as 1.0.0 though" is the mistake we made with GraphQL and I'd rather not make it again. If the API is stable for 6-12 months and I release 1.0 without breakages, it will be easy to update framework to support In the meantime tying to a 0.1 release says 'this API is new, watch out' which I think is appropriate. If we start getting pen-test reports complaining about it or something, we can always look to revise that decision, even in a framework patch release. Does that rationale seem reasonable to you? TL;DR: tagging 0.1.0 now. |
|
FYI before tagging 0.1.0 I added a couple of things that I had intended to do. They don't really relate to this use-case:
|
This provides a mechanism for adjusting the behaviour of these
relations when building more complex data models.
For example the following example has a status field incorporates a
Status field into the relationship:
```php
function MyRelation() {
$rel = $this->getManyManyComponents(‘MyRelation’);
$rel = $rel->filter(‘Status’, ‘Active’);
$rel->addCallbacks()->add(function ($relation, $item, $extra) {
$item->Status = ‘Active’;
$item->write();
});
}
```
Introduces a new library dependency: http://github.com/sminnee/callbacklist
|
Sweet, yeah 0.1.0 is fine. Changed constraint to |
This provides a mechanism for adjusting the behaviour of these
relations when building more complex data models.
For example the following example has a status field incorporates a
Status field into the relationship:
to do