Added logic to LanguageController for supporting Relation::morphMap([...])
#6
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/Http/Controllers/LanguageController.php (2)
Line range hint
21-40: Optimize model instantiation to improve performance.The code instantiates the same model multiple times using
app($request->get('model')). This is inefficient and violates the DRY principle.Consider refactoring to store the model instance in a variable:
if (Schema::hasColumn(app($request->get('model'))->getTable(), 'lang')) { - $model = app($request->get('model'))->find($request->get('model_id')); + $modelClass = app($request->get('model')); + $model = $modelClass->find($request->get('model_id')); $model->lang = $request->get('lang'); $model->save(); } else { + $modelClass = app($request->get('model')); + $morphClass = $modelClass->getMorphClass(); $lang = UserLanguage::query() - ->where('model_type', app($request->get('model'))->getMorphClass()) + ->where('model_type', $morphClass) ->where('model_id', $request->get('model_id')) ->first(); if ($lang) { $lang->lang = $request->get('lang'); $lang->save(); } else { UserLanguage::query()->create([ - 'model_type' => app($request->get('model'))->getMorphClass(), + 'model_type' => $morphClass, 'model_id' => $request->get('model_id'), 'lang' => $request->get('lang'), ]); } }
Line range hint
21-22: Add error handling for invalid model class.The code doesn't validate if the provided model class exists before attempting to instantiate it with
app(). This could lead to uncaught exceptions.Consider adding validation:
+ try { + $modelClass = app($request->get('model')); + } catch (\Throwable $e) { + Notification::make() + ->title('Invalid model class provided') + ->icon('heroicon-o-x-circle') + ->iconColor('danger') + ->send(); + return back(); + } + if (Schema::hasColumn(app($request->get('model'))->getTable(), 'lang')) {tests/src/LanguageSwitcherTest.php (2)
65-81: Consider reducing test duplication through shared setupWhile the test implementation is correct, there's significant code duplication with the 'can switch language to ar' test. Consider extracting the common test logic into a shared function or data provider.
Example refactor:
function assertLanguageSwitch($lang, $useMorphMap = false) { if ($useMorphMap) { Relation::morphMap(['user' => get_class(auth()->user())]); } $response = get(route('languages.switcher', [ 'model' => get_class(auth()->user()), 'model_id' => auth()->user()->id, 'lang' => $lang, ])); $response->assertStatus(302); expect(auth('web')->user()->lang)->toBe($lang); } it('can switch language to ar', fn() => assertLanguageSwitch('ar')); it('can switch language with morphMap', fn() => assertLanguageSwitch('ar', true));
69-73: Inconsistent auth guard usageThe test uses
auth()->user()for retrieving the model and ID, butauth('web')->user()for verifying the language. Consider using consistent auth guard access throughout the test.- 'model' => get_class(auth()->user()), - 'model_id' => auth()->user()->id, + 'model' => get_class(auth('web')->user()), + 'model_id' => auth('web')->user()->id,Also applies to: 78-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Http/Controllers/LanguageController.php(2 hunks)tests/src/LanguageSwitcherTest.php(2 hunks)
🔇 Additional comments (2)
src/Http/Controllers/LanguageController.php (1)
27-27: LGTM! Proper implementation of morphMap support.
The changes correctly implement support for morphMap by using getMorphClass() instead of the raw model string. This ensures proper polymorphic relationship mapping when models use Laravel's morphMap feature.
Also applies to: 36-36
tests/src/LanguageSwitcherTest.php (1)
3-3: LGTM! Import statement is correctly added
The import for Relation class is necessary for the new morphMap functionality being tested.
This PR includes a minor addition which allows the package to be used when a MorphMap is used on a model with
InteractsWithLanguages.I've chosen to use the already existing syntax of
app($request->get('model')). So no other logic had to be changed.That being said, if you prefer to have it as variable (i.e.
$model = app($request->get('model'))) because it is now repeated four times in the controller, I don't mind changing it.Summary by CodeRabbit
New Features
Tests