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

PHPORM-222 Register the BusServiceProvider when BatchRepository is built #3071

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jul 23, 2024

Fix PHPORM-222
Fix #3060

MongoDBBusServiceProvider is not the provider of BatchRepository.
I could not reproduce the issue described in #3060, but it looks like this change should fix it.

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

@GromNaN GromNaN added the bug label Jul 23, 2024
@GromNaN GromNaN added this to the 4.7 milestone Jul 23, 2024
@GromNaN GromNaN requested a review from a team as a code owner July 23, 2024 16:47
@GromNaN GromNaN requested a review from alcaeus July 23, 2024 16:47
@GromNaN GromNaN mentioned this pull request Jul 23, 2024
alcaeus
alcaeus previously approved these changes Jul 23, 2024
@degecko
Copy link

degecko commented Jul 24, 2024

Correct me if I'm wrong, but doesn't this fix render the code from the register method useless?

If I'm correct in assuming that the idea was to extend the BatchRepository service provider for the case where the queue batching driver was set to mongodb, then the proposed solution will invalidate that solution.

It's also true that the way the BatchRepository resolver has been extended never actually worked. I think that's the actual bug that needs fixing.

@GromNaN
Copy link
Member Author

GromNaN commented Jul 24, 2024

Ok, I think I've found how this should be done.

Illuminate\Bus\BusServiceProvider is a DeferrableProvider. It's registered only when one of the services it provides is requested. But by extending this service in my provider, I replace the service provider that is registered. I with Laravel would call every providers that provide a given service, but there is only one.
I need to explicitly register the Illuminate\Bus\BusServiceProvider when my provider is registered.

@alcaeus alcaeus dismissed their stale review July 24, 2024 11:57

Implementation changed

@alcaeus alcaeus self-requested a review July 24, 2024 11:57
@GromNaN GromNaN changed the title PHPORM-222 MongoDBBusServiceProvider does not provide BatchRepository PHPORM-222 Register the BusServiceProvider when BatchRepository is built Jul 24, 2024
@GromNaN
Copy link
Member Author

GromNaN commented Jul 24, 2024

Ideally that could be solved in Laravel: laravel/framework#52251

@GromNaN GromNaN merged commit 895dcc7 into mongodb:4.7 Jul 25, 2024
26 checks passed
@GromNaN GromNaN deleted the PHPORM-222 branch July 25, 2024 09:21
This was referenced Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants