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

Upgrade RouteActionCallableRector rule to handle groups #284

Merged
merged 10 commits into from
Jan 7, 2025

Conversation

peterfox
Copy link
Collaborator

@peterfox peterfox commented Jan 3, 2025

Changes

  • Adds new scenarios to the RouteActionCallableRector rule to cover groups
  • Adds new methods to RouterRegisterNodeAnalyzer
  • Adds tests covering the scenarios added
  • Updates the docs

Why

Resolves #178

-Route::get('/users', 'UserController@index');
+Route::get('/users', [\App\Http\Controllers\UserController::class, 'index']);

 Route::group(['namespace' => 'Admin'], function () {
-    Route::get('/users', 'UserController@index');
+    Route::get('/users', [\App\Http\Controllers\Admin\UserController::class, 'index']);
 })

This also fixes a bug as such where previously routes within a group with a namespace would be rewritten by the rule but given the wrong namespace. This likely didn't occur often because the rule only applies a change if the controller can be found as an existing class.

Notes

Removing Group info

This doesn't remove the namespace from the group. Right now that wouldn't cause a problem because when the controller fully qualified class name is provided to the router, it overrides the group namespace. It would be challenging to implement a removal of the namespace from the group if one or more routes can't be updated within a group.

Another rule that removes the property group etc. might be best to avoid over stuffing this rule anyways.

Groups with the Namespace call

currently:

Route::namespace('Admin')->group(['namespace' => 'Admin'], function () {
    Route::get('/users', 'UserController@index');
    Route::get('/users', [\App\Http\Controllers\Admin\UserController::class, 'index']);
 })

Won't be converted. It shouldn't be too hard to make this happen, but I wanted to get an MVP of the rule with groups merged in first.

Adding to Laravel 11 Upgrades

This rule should ideally be added to Laravel 11's upgrade rules. I wasn't aware of this because I never used namespaces once Laravel introduced the fully qualified names, but Laravel 11 does away with namespaces, making this rule a valuable upgrade option over having to fiddle with routes.

@peterfox peterfox added the enhancement New feature or request label Jan 3, 2025
@peterfox peterfox self-assigned this Jan 3, 2025
@GeniJaho
Copy link
Collaborator

GeniJaho commented Jan 5, 2025

@peterfox let's add it to the Laravel 11 rules, please.
I think we should add more rules to the level sets since they are supposedly used mostly during one-time upgrades.

@peterfox peterfox requested a review from GeniJaho January 6, 2025 21:10
@peterfox
Copy link
Collaborator Author

peterfox commented Jan 6, 2025

@GeniJaho I've added it to the Laravel 8 set. Based on the docs https://laravel.com/docs/8.x/upgrade#automatic-controller-namespace-prefixing

The only slight issue I can see is that the namespace is set to be \App\Https\Controllers in the config by default and being in a rule set there's no way to override them. I think the only way I can resolve that is with something weird like a static variable.

@GeniJaho
Copy link
Collaborator

GeniJaho commented Jan 7, 2025

You're right, there's no easy way to include this rule by default, I missed that case 😅

Should we just keep it out of the set until we find a good solution?

Also are the tests failing because of changes in core Rector?

@peterfox
Copy link
Collaborator Author

peterfox commented Jan 7, 2025

@GeniJaho yes, those tests are failing in the main branch. I've added a PR that fixes them #285

@peterfox
Copy link
Collaborator Author

peterfox commented Jan 7, 2025

@GeniJaho as for the set. I think personally for now, let's remove it from the set and consider it in another PR.

This reverts commit f9c86cf.

# Conflicts:
#	tests/Sets/Laravel80/config/configured_rule.php
@GeniJaho GeniJaho merged commit fc3c4d6 into main Jan 7, 2025
5 checks passed
@GeniJaho GeniJaho deleted the feature/upgrade-route-rule-2 branch January 7, 2025 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Route::group() with namespace doesnt work with RouteActionCallableRector
2 participants