-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
refactor: extract DefinedRouteCollector #7653
refactor: extract DefinedRouteCollector #7653
Conversation
d12c63b
to
6f0d950
Compare
/** | ||
* Collect all defined routes for display. | ||
*/ | ||
class DefinedRouteCollector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this intended to be extended? can this be made final
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
return (new RouteCollection($loader, $moduleConfig, new Routing()))->setHTTPVerb('get'); | ||
} | ||
|
||
public function test() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this test name descriptive. If using --test-dox
option this test will have an empty name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to rename it. Done.
$this->routeCollection = $routes; | ||
} | ||
|
||
public function collect(): Generator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the signature of the Generator in the PHPDoc so that it can be used by IDEs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sig begs the question: is it worth introducing a value object at some point for better definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data structure of a route is complex, so it would be easier to understand if it were a class.
PHPStan is unhappy. |
See #7649 |
in addition to this when a new RouteCollection library is created implementing the RouteCollectionInterface this throw an exception of CodeIgniter\Router\DefinedRouteCollector::__construct(): Argument #1 ($routes) must be of type CodeIgniter\Router\RouteCollection, App\Libraries\Routing\RouteCollection given, called in C:\laragon\www\sebaadev\system\Debug\Toolbar\Collectors\Routes.php on line 103 @kenji I suggest you replace I edited the core code of DefinedRouteCollector to this and it worked private RouteCollectionInterface $routeCollection;
public function __construct(RouteCollectionInterface $routes)
{
$this->routeCollection = $routes;
} |
@ChibuezeAgwuDennis We can't. Because |
Okay i understand but it can be fixed since |
Description
Checklist: