Issue Event for Row Rendering

I think this approach makes sense. I'm not sure if it's worth having the module itself provide the "starter" event subsubscriber to provide all the initial data for the row or not. I don't think it's a super common pattern to do that, rather the module would still have its core business logic present to get those starter values, then dispatch an event with those values to allow other modules to alter them. Though, this method does make it easier for some other module to completely disable the main event subscriber if they want.

Are the module maintainers on board with an approach like this? If so, maybe we can get some tests written.

I don't think it's a super common pattern to do that, rather the module would still have its core business logic present to get those starter values, then dispatch an event with those values to allow other modules to alter them. Though, this method does make it easier for some other module to completely disable the main event subscriber if they want.

I agree that it's not super common, but the reason I chose to do this here was flexibility -- like you said to disable the main subscriber if desired. You could also disable the main event subscriber and override the "starter" subscriber if your use case allows for it.

Thank you for creating this and for the MR.
I think it's OK to open an integration point and allow modules to alter the row contents. However, as mentioned above, I would rather have the current code do its thing and just dispatch the event (or even a good ol' alter hook, it works fine for me), so other modules can alter the contents of the generated row. I could see the most common use for this would be to tweak things a bit, not re-writing everything entirely. (maybe if projects need to do something completely different it would be easier to just take over the controller at that point?)

In my mind with this new hook/event there would be a small performance penalty to all sites, for the benefit of the ones that do need to alter something, but I believe the trade-off is worth it.

Thanks,

I could see the most common use for this would be to tweak things a bit, not re-writing everything entirely. (maybe if projects need to do something completely different it would be easier to just take over the controller at that point?)

Agreed .. I just thought that it made the most sense to provide the module's default event implementation using the same mechanism provided to anyone else who might be interested. Of course, the original way works too and I can see merit to that as often there's just a tweak needed by a consuming module. FWIW, doing it this way helped me identify an appropriate place to fire the event.

Just please let me know if, after considering this, you'd still like to have the generated row implementation in the controller.

Just please let me know if, after considering this, you'd still like to have the generated row implementation in the controller.

Yes, I think this would be the preferred approach, for maintainability reasons at this point. Thanks! 👍

@partyka thanks for working on this. The MR is still marked as Draft, can you please remove that when it's ready for review?
(also, there seems to be a lot of changes in the controller code that may not be related to this change? It would be great if we could remove those to make reviewing easier)
Thanks!

bkosborne → changed the visibility of the branch 3396987-issue-event-for to hidden.

bkosborne → changed the visibility of the branch 3396987-issue-event-for to active.