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

Would you consider approaching Laravel to include this in core? #32

Closed
mfn opened this issue Sep 26, 2019 · 5 comments
Closed

Would you consider approaching Laravel to include this in core? #32

mfn opened this issue Sep 26, 2019 · 5 comments

Comments

@mfn
Copy link
Contributor

mfn commented Sep 26, 2019

I'm aware the history of the project originated in trying to have this in Laravel.

Personally the approach used feels very clean and it definitely is production ready.

Also by using the explicit interface to mark events, the addition to Laravel core could be done in almost backwards compatible way (i.e. it could start without the white/blacklist thing), i.e. changing at least no runtime behaviour (as long as users didn't replace it with their own event bus, etc.).

What's your take on this thought?

@fntneves
Copy link
Owner

Hello @mfn,

I consider this behavior could be easily integrated in Laravel core.
Similarly to the ShouldQueue interface, the recommended approach would be implementing TransactionalEvent to mark events as transactional.

Actually, I do not like the approach of changing a configuration file to achieve logic behavior. Therefore, I'm OK in removing such white/blacklist thing.

How do you suggest to approach this?

@mfn
Copy link
Contributor Author

mfn commented Sep 27, 2019

Thanks for stating your position, sounds good 😄

I'll think more about it, briefly said: there was a lengthy (old-ish) idea laravel/ideas#1441 which recently had a bit traction but was then closed. I tried to expose the usefulness of this library but obviously failed to do so.

@mfn
Copy link
Contributor Author

mfn commented Oct 11, 2019

My company is also a Nova customer and were hit with the exactly same problem there too, as did others laravel/nova-issues#1759

Just mentioned this here because also customers of official commercial Laravel solutions are affected by the lack of transactional events in the core and people are recommending to use this package.

@mfn
Copy link
Contributor Author

mfn commented Nov 1, 2019

How do you suggest to approach this?

I suggest to:

  • start a PR against Laravel code base but which is first only review discussed here so we're sure it's an implementation we like
  • I would not, at this point, open a new laraval/idea issue; there have been already some in the past ( https://github.com/laravel/ideas/issues?utf8=%E2%9C%93&q=is%3Aissue+transaction+event ); they were not closed because of an implementation or for being turned down; some just weren't satisfied with the state of discussion and moved on
  • open an official PR for discussion and immediate testing by the community, laying out the philosophy / underlying approach

It's important for the PR and the signal we want to send that it's in a production-ready state, all tests pass and additional tests have been added.

In short: a bit of work. I'm willing to do that. However it's also important for me that I don't get "commiter credit" for this; I estimate it won't be much original code but simply assimilating this library. I'm sure we'll find a solution for this, just wanted to make this clear upfront :)

@mfn
Copy link
Contributor Author

mfn commented Apr 12, 2020

Hello everyone watching this issue!

I finally found time and got around to start working on this => as I suggested previously:

start a PR against Laravel code base but which is first only review discussed here so we're sure it's an implementation we like

Please head over to mfn/laravel-framework#1 for all your feedback, thanks!

@fntneves fntneves closed this as completed Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants