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

Add other model's relation events #4561

Closed
Rike-cz opened this issue Aug 22, 2019 · 12 comments
Closed

Add other model's relation events #4561

Rike-cz opened this issue Aug 22, 2019 · 12 comments

Comments

@Rike-cz
Copy link
Contributor

Rike-cz commented Aug 22, 2019

By now there are only attach/detach events for BelongsToMany relation. Adding other events will make using OctoberCMS more comfortable. Good example there: https://github.com/chelout/laravel-relationship-events.
Please add this feature request to bounty.

@LukeTowers
Copy link
Contributor

Please only use GitHub Issues for feature requests if you are using it as a pre-Pull Request collaboration tool. If you aren't planning on implementing it yourself, then it would be better suited being asked for on one of these channels:

Forum: http://octobercms.com/forum
IRC: #october on Freenode or http://octobercms.com/chat
Slack: https://octobercms-slack.herokuapp.com/

The support demand for October is increasing, while the available time the founders have to give remains the same. Feature requests are by default the lowest priority for us, as we don't have time to implement everyone's suggestions by ourselves. However there are ways you can help:

  • Submit a pull request that introduces this feature and we'll review it. Please note that the amount of effort making the PR (code quality, rationale for inclusion in the project, explanation of changes) is directly proportional to the likelihood that we will review and merge it.
  • Make a financial contribution to the project so we can allocate more time towards making improvements like these that benefit everyone

@Rike-cz
Copy link
Contributor Author

Rike-cz commented Aug 24, 2019

More details

Just now we have this 4 relation events for BelongsToMany relation:

  • model.relation.beforeAttach (string $relationName, array $attachedIdList, array $insertData),
  • model.relation.afterAttach (string $relationName, array $attachedIdList, array $insertData),
  • model.relation.beforeDetach (string $relationName, array $attachedIdList),
  • model.relation.afterDetach (string $relationName, array $attachedIdList).

But other events and relations are not covered. This is table of full covered relations and their events:

hasOneOrMany

Events Params
model.relation.beforeCreate string $relationName, Model $related
model.relation.afterCreate string $relationName, Model $related
model.relation.beforeUpdate string $relationName, Model $related
model.relation.afterUpdate string $relationName, Model $related
model.relation.beforeSave string $relationName, Model $related
model.relation.afterSave string $relationName, Model $related

belongsTo

Events Params
model.relation.beforeUpdate string $relationName, Model $related
model.relation.afterUpdate string $relationName, Model $related
model.relation.beforeAssociate string $relationName, Model $related
model.relation.afterAssociate string $relationName, Model $related
model.relation.beforeDissociate string $relationName, Model $related or int $relatedId
model.relation.afterDissociate string $relationName, Model $related or void

belongsToMany

Events Params -
model.relation.beforeAttach string $relationName, array $attachedIdList, array $insertData Existing, needs to be fired on BelongsToMany::save
model.relation.afterAttach string $relationName, array $attachedIdList, array $insertData Existing, needs to be fired on BelongsToMany::save
model.relation.beforeDetach string $relationName, array $attachedIdList Existing
model.relation.afterDetach string $relationName, array $attachedIdList Existing
model.relation.beforeSync string $relationName, array $detachedIdList, $insertData fired also on BelongsToMany::attach, BelongsToMany::detach, maybe redundant
model.relation.afterSync string $relationName, array $detachedIdList, $insertData fires also on BelongsToMany::attach, BelongsToMany::detach, maybe redundant
model.relation.beforeToggle string $relationName, array $detachedIdList, $insertData fires also on BelongsToMany::attach, BelongsToMany::detach, maybe redundant
model.relation.afterToggle string $relationName, array $detachedIdList, $insertData fires also on BelongsToMany::attach, BelongsToMany::detach, maybe redundant
model.relation.beforeUpdateExistingPivot string $relationName, int $relatedId, $insertData
model.relation.afterUpdateExistingPivot string $relationName, Model $related, $insertData

For polymorphic relations is situation the same. MorphToMany a MorphedByMany like belongsToMany.
If returning the Model needs new query, primary key will be enough. Events are derived from Laravel framework. I see that October has some specific relation helpers (HasOneOrMany::add, HasOneOrMany::remove...). I do not know is extending events granularity is good solution, maybe new Model::bindEvents(['model.afterCreate', 'model.afterDelete'], function () ...) would help.

Why?

I think this feature was incomplete. BelongsToMany event was covered only for attaching and detaching, but belongsTo not. My usecases need especially trigger event when model is created from side of parent (hasOne, hasMany), but this is not covered at all. Adding this events help to make code more clarity on side Model relations instead of controller side.

@LukeTowers
Copy link
Contributor

@Rike-cz could you post an example of how you plan on using these events in your project / plugins?

@Rike-cz
Copy link
Contributor Author

Rike-cz commented Aug 24, 2019

In model constructor for example:

class Product extends Model
{
    public function __construct(array $attributes = [])
    {
        parent::__construct($attributes);

        $this->bindEvent('model.relation.afterAttach', function ($relationName, $attachedIdList, $insertData) {
            if ($relationName == 'files') {
                $this->recountRelationSortableOrder('files'); // I have custom sorting algorithm
            }
        });

        $this->bindEvent('model.relation.afterAssociate', function ($relationName, $related) {
            if ($relationName == 'file') {
                $related->touchAssociated(); // touch file when associated
            }
        });

        $this->bindEvent('model.relation.beforeSave', function ($relationName, $related) {
            if ($relationName == 'files') {
                if ($this->... && $related->...) throw \Exception('Cannot make relation between this models!'); // something like relation validation
            }
        });
    }
...
}

@github-actions
Copy link

github-actions bot commented Oct 1, 2019

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.

@Rike-cz
Copy link
Contributor Author

Rike-cz commented Oct 27, 2020

After the time I decided to make revision of this conceptual enhancement. For me looks before/after create/update/save events redundant now, because can be simply handle by relationed model as usual. Only these events looks as benefit for handling birth and death of relationships:

BelongsToMany

Events Params -
model.relation.beforeAttach string $relationName, array $attachedIdList, array $insertData Existing
model.relation.afterAttach string $relationName, array $attachedIdList, array $insertData Existing
model.relation.beforeDetach string $relationName, array $attachedIdList Existing
model.relation.afterDetach string $relationName, array $attachedIdList Existing

BelongsTo, MorphTo

Events Params
model.relation.beforeAssociate string $relationName, Model $related
model.relation.afterAssociate string $relationName, Model $related
model.relation.beforeDissociate string $relationName, Model $related or int $relatedId
model.relation.afterDissociate string $relationName, Model $related or void

HasOneOrMany, AttachOneOrMany, MorphOneOrMany

And here is the question. To have inversional events for these relationships or to call inversional counterparts above when needed? When needed means add/remove methods where relationship arises.
If is it good idea to have them here too (I don't think so now), then:

Events Params
model.relation.beforeAdd string $relationName, Model $related
model.relation.afterAdd string $relationName, Model $related
model.relation.beforeRemove string $relationName, Model $related
model.relation.afterRemove string $relationName, Model $related

Handling pivots is possible via handling pivot models as needed (MorphPivot will come soon). I think there are no additional events needed.

@LukeTowers
Copy link
Contributor

Why are there three different types of events instead of just one?

@Rike-cz
Copy link
Contributor Author

Rike-cz commented Oct 29, 2020

They are called by their callers. Existing BelongsToMany {after|before}Attach event was also called by the method, that caused relation change. Can you elaborate your purpose? I am not sure if I understand.

@LukeTowers
Copy link
Contributor

I mean why wouldn't the event name just be model.relation.before/after attach/detach for all of the relation types?

@LukeTowers
Copy link
Contributor

Are you saying that Laravel already has beforeAttach() model methods that are called in those cases?

@Rike-cz
Copy link
Contributor Author

Rike-cz commented Oct 29, 2020

Not Laravel. OctoberCMS has this event: https://octobercms.com/docs/api/model/relation/afterattach. I thought that good way is to respect naming convention, so Model::belongsTo()::associate() fires event with this name but it is not necessary. This Laravel package was my earlier inspiration: https://github.com/chelout/laravel-relationship-events.

@LukeTowers
Copy link
Contributor

hmm, fair enough. I'm fine with those names then.

@github-actions github-actions bot closed this as completed Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants