Sylius: [RFC] Better naming for admin events

[pjedrzejewski]

Hi folks!

By default, ResourceController fires sylius.<resourceName>.<actionName> event for all actions. I think we should have a bit more specific naming strategy for admin, so that we can be sure the listener is only for admin. Something like sylius.product.admin.pre_create? What do you think?

[sweoggy]

Sounds great, but I believe this introduces a BC break? Not sure it can be considered to be a "major DX issue".

[tchapi]

My two cents: I think the actual behavior is quite satisfactory — when you make use of the events, for, let's say, create a log when some resource is created or updated, it's generally agnostic of the _interface_ you use to trigger the event.

If we were to follow your specific naming, then there would be a sylius.product.api.pre_create too, and then it's becoming a real _anti-pattern_.

In our use case, I'm pretty happy with the actual behavior. We have resources (Training) that we create, either in the frontend (by the customers) or the backend or api (the admins), and we use the events to create products (TrainingSession) that find their way in the shop for other customers to buy. Whether the backend or the frontend is used makes no difference : the TrainingSession object is _inherently_ linked to the Training object, and in that way it is inseparable, thus using the same event to create the relevant product seems consistent and perfectly normal to us.

[pjedrzejewski]

@tchapi @sweoggy Thanks for your feedback guys, good points! Let's hear few more opinions and decide what to do. :)

[gabiudrescu]

sometimes, you want to have different behavior between creation from shop and creation from admin / API. at bestvalue.eu, when a customer is adding a planeTicket to this account, we want him to receive an specific type of email.

but when a customer care representative does this from admin, we want him to receive a different kind of email. or none at all.

or - if you use the API for native mobile apps - you could use this event source as a hook to trigger different behavior between environments.

---

Though, if I am 100% honest to myself and think a little bit more in terms of YAGNI (you ain't gonna need it), this is a _nice to have_ feature.

It would be best to have events as granular as possible, but if this brings lots of BCs and there are no strong business value to the table, then I vote for "NO" - do not implement this right now.

[michalmarcinkowski]

IMO the default event is good enough. If someone needs to differentiate between e.g. admin/shop actions he still can pass custom event name in the route options for these specific actions. I think it's not so common use case that we need a better build in support for that.

[crevillo]

i'm not against the separation but i will definitely keep the default behaviour in place. saying that because if separation is done, will devs will need to write both definitions if they want some event be applied in admin and shop?

[isometriks]

How about keeping the event names the same and just adding a context to the events?

if ($event->getContext() !== ResourceActions::CONTEXT_BACKEND) {
    return;
}

// do something for admin only event

Use backend, frontend, and even API if needed. Can still keep BC easily and then you won't have to subscribe to multiple events if you always want it to happen.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.