feat: register service providers to providers.go[#790]#1248
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates service provider registration from config/app.go to bootstrap/providers.go across all framework setup files. The change adds a new BootstrapPath method to the Application interface and updates all service installation/uninstallation code to use the new location.
Key changes:
- Added
BootstrapPathmethod to Application interface and implementation - Updated all setup files to register providers in
bootstrap/providers.goinstead ofconfig/app.go - Introduced new
Providers()matcher function and deprecated the oldProvidersFallback()for backward compatibility
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/foundation/application.go | Added BootstrapPath method to Application interface |
| foundation/application.go | Implemented BootstrapPath method to return bootstrap directory path |
| support/path/path.go | Added Bootstrap helper function wrapping BootstrapPath |
| packages/match/helper.go | Added new Providers() matcher and deprecated ProvidersFallback() |
| packages/match/helper_test.go | Updated tests to use new provider registration pattern |
| mocks/foundation/Application.go | Added mock implementation for BootstrapPath method |
| auth/setup/setup.go | Updated to use providersBootstrapPath and fixed typo in modulePath variable |
| cache/setup/setup.go | Updated to use providersBootstrapPath and extracted path variables |
| crypt/setup/setup.go | Updated to use providersBootstrapPath and facade constants |
| database/setup/setup.go | Updated to use providersBootstrapPath |
| event/setup/setup.go | Updated to use providersBootstrapPath |
| filesystem/setup/setup.go | Updated to use providersBootstrapPath and facade constants |
| http/setup/setup.go | Updated to use providersBootstrapPath |
| log/setup/setup.go | Updated to use providersBootstrapPath |
| mail/setup/setup.go | Updated to use providersBootstrapPath |
| process/setup/setup.go | Updated to use providersBootstrapPath |
| queue/setup/setup.go | Updated to use providersBootstrapPath |
| route/setup/setup.go | Updated to use providersBootstrapPath |
| schedule/setup/setup.go | Updated to use providersBootstrapPath |
| session/setup/setup.go | Updated to use providersBootstrapPath |
| testing/setup/setup.go | Updated to use providersBootstrapPath |
| translation/setup/setup.go | Updated to use providersBootstrapPath and facade constants |
| validation/setup/setup.go | Updated to use providersBootstrapPath and facade constants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
filesystem/setup/setup.go
Outdated
| modify.GoFile(path.Config("app.go")). | ||
| Find(match.Providers()).Modify(modify.Unregister("&filesystem.ServiceProvider{}")). | ||
| modify.WhenNoFacades([]string{facades.Storage}, | ||
| modify.GoFile(providersBootstrapPath). |
There was a problem hiding this comment.
@hwbrzzl The install and uninstall logic seems redundant. Should we move these to packages as InstallServiceProvider and UninstallServiceProvider? In the case of uninstall, I now have to fall back to the previous logic as well.
There was a problem hiding this comment.
Yes, InstallServiceProvider and UninstallServiceProvider are fine, and we only need to implement registering ServiceProvider in the providers.go file. It's compatible with the old style.
There was a problem hiding this comment.
That’s already implemented, it’s just that during uninstallation, we first need to remove the entry from bootstrap/providers.go, and then from config/app.go as a fallback for older applications.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1248 +/- ##
==========================================
+ Coverage 68.19% 68.42% +0.22%
==========================================
Files 264 264
Lines 15425 15483 +58
==========================================
+ Hits 10519 10594 +75
+ Misses 4449 4425 -24
- Partials 457 464 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
auth/setup/setup.go
Outdated
| Install( | ||
| modify.GoFile(appConfigPath). | ||
| Find(match.Imports()).Modify(modify.AddImport(modulepath)). | ||
| modify.GoFile(providersBootstrapPath). |
There was a problem hiding this comment.
Will you implement InstallServiceProvider and UninstallServiceProvider functions in this PR, please? The basic logic of them should be: Check if the bootstrap/provider.go exists, if so, add the service provider into provider.go, if not, add the service provider into config/app.go. The same as UninstallServiceProvider
There was a problem hiding this comment.
Hmm, I believe the logic should ideally be this: if providers.go doesn’t exist, then create it and add the provider(this should also add .WithProviders(Providers()) automatically). Otherwise, we won’t be able to deprecate the old one. In the case of uninstall, we should first try to uninstall from providers.go and then from the config as a fallback. This way, we establish the habit that new providers should go into providers.go, and the user can either migrate app.providers to providers.go or maintain both.
There was a problem hiding this comment.
The old project may not use foundation.Configure, so creating providers.go is unnecessary.
There was a problem hiding this comment.
providers.go should exist in the new project by default, app_service_provider should be registered there.
There was a problem hiding this comment.
Hmm, make sense, but in case of uninstall regardless of providers.go should I try to delete from both the file?
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: da8a5c4 | Previous: 100c50f | Ratio |
|---|---|---|---|
Benchmark_DecryptString |
6551 ns/op 2032 B/op 16 allocs/op |
2109 ns/op 2032 B/op 16 allocs/op |
3.11 |
Benchmark_DecryptString - ns/op |
6551 ns/op |
2109 ns/op |
3.11 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Hey @kkumar-gcc This PR is blocking me, let me optimize it directly. Thanks for the completed logic. |
82316e0 to
da8a5c4
Compare
da8a5c4 to
08a025a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @kkumar-gcc This PR has been completed. Could you review it if you have time? I'll merge it if you don't have any concerns. Thanks! |
krishankumar01
left a comment
There was a problem hiding this comment.
LGTM, just a couple of doubts
|
|
||
| func (r *Application) BootstrapPath(path ...string) string { | ||
| path = append([]string{support.RelativePath, "bootstrap"}, path...) | ||
| return internals.Abs(path...) |
There was a problem hiding this comment.
Should we use r.Path here?
There was a problem hiding this comment.
r.Path is app/**, so it cannot be used here.
| return WrapNewline(expr) | ||
| } | ||
|
|
||
| // RemoveProvider removes a service provider from the foundation.Setup() chain in the Boot function. |
There was a problem hiding this comment.
Are we moving towards GoDoc-style comments? Though most of these feels redundant.
There was a problem hiding this comment.
The logic is a bit complicated, so the comment is necessary I think, and it's also used by AI, it should not be a big deal.
| } | ||
|
|
||
| // Remove from inline array in app.go | ||
| if err := GoFile(r.appFilePath).Find(match.FoundationSetup()).Modify(r.removeInline(item)).Apply(); err != nil { |
There was a problem hiding this comment.
Will this also remove provider from config/app.go?
There was a problem hiding this comment.
The current goravel/goravel structure doesn't support package:install/uninsatll facade, so I think it's unnecessary to remove provider from config/app.go.
📑 Description
Closes https://github.com/goravel/goravel/issues/
✅ Checks
Greptile Overview
Greptile Summary
This PR migrates service provider registration from
config/app.goto a dedicatedbootstrap/providers.gofile, implementing a cleaner separation of concerns for framework bootstrapping.Key Changes:
AddProvider()andRemoveProvider()functions to programmatically manage service provider registrationRemoveItem()method inwithSliceHandlerfor removing items from bootstrap slices with proper import cleanupBootstrapPath()method to the Application interface and implementationAddProviderApply()andRemoveProviderApply()patternmake:providercommand to automatically register new providers inbootstrap/providers.goProvidersInConfig()matcher in favor of newProviders()matcher that targets the bootstrap fileImpact:
Confidence Score: 4/5
Important Files Changed
RemoveItemmethod to remove providers/commands/etc from bootstrap files, with proper import cleanupAddProviderandRemoveProviderfunctions with comprehensive documentationAddProviderApplyandRemoveProviderApplywrapper functions with newCallmodifierBootstrapPathmethod to get path to bootstrap directorybootstrap/providers.goProviders()matcher for new pattern, deprecated oldProvidersInConfig()Sequence Diagram
sequenceDiagram participant User participant MakeCommand as make:provider Command participant ModifyPkg as modify Package participant BootstrapApp as bootstrap/app.go participant ProvidersFile as bootstrap/providers.go User->>MakeCommand: Run make:provider MyProvider MakeCommand->>MakeCommand: Create provider file MakeCommand->>ModifyPkg: AddProvider(pkg, provider) alt providers.go doesn't exist AND WithProviders not in Setup ModifyPkg->>ProvidersFile: Create providers.go with stub template ModifyPkg->>ProvidersFile: Add provider to Providers() function ModifyPkg->>BootstrapApp: Add WithProviders(Providers()) to Setup chain else providers.go exists AND WithProviders in Setup ModifyPkg->>ProvidersFile: Add provider to existing Providers() function ModifyPkg->>ProvidersFile: Add import if needed else providers.go doesn't exist BUT WithProviders exists inline ModifyPkg->>BootstrapApp: Add provider to inline []foundation.ServiceProvider array ModifyPkg->>BootstrapApp: Add import if needed end ModifyPkg-->>MakeCommand: Success MakeCommand-->>User: Provider registered successfully