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

refactor: extract DefinedRouteCollector #7653

Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 4, 2023

Description

  • extract DefinedRouteCollector

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

@kenjis kenjis added refactor Pull requests that refactor code 4.4 labels Jul 4, 2023
@kenjis kenjis force-pushed the refactor-extract-DefinedRouteCollector branch from d12c63b to 6f0d950 Compare July 4, 2023 03:03
@kenjis kenjis mentioned this pull request Jul 4, 2023
5 tasks
/**
* Collect all defined routes for display.
*/
class DefinedRouteCollector
Copy link
Member

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?

Copy link
Member Author

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()
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

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?

Copy link
Member Author

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.

@MGatner
Copy link
Member

MGatner commented Jul 4, 2023

PHPStan is unhappy.

@kenjis
Copy link
Member Author

kenjis commented Jul 4, 2023

PHPStan is unhappy.

See #7649

@kenjis kenjis merged commit 8d9b176 into codeigniter4:4.4 Jul 4, 2023
@kenjis kenjis deleted the refactor-extract-DefinedRouteCollector branch July 4, 2023 22:53
@ChibuezeAgwuDennis
Copy link

ChibuezeAgwuDennis commented Oct 2, 2023

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 CodeIgniter\Router\RouteCollection to this CodeIgniter\Router\RouteCollectionInterface and use more of Services::routes()

I edited the core code of DefinedRouteCollector to this and it worked

    private RouteCollectionInterface $routeCollection;

    public function __construct(RouteCollectionInterface $routes)
    {
        $this->routeCollection = $routes;
    }

@kenjis
Copy link
Member Author

kenjis commented Oct 2, 2023

@ChibuezeAgwuDennis We can't. Because RouteCollectionInterface is broken.
It does not have getRoutesOptions().
To do so, we need to fix RouteCollectionInterface first.

@ChibuezeAgwuDennis
Copy link

ChibuezeAgwuDennis commented Oct 3, 2023

@ChibuezeAgwuDennis We can't. Because RouteCollectionInterface is broken.
It does not have getRoutesOptions().
To do so, we need to fix RouteCollectionInterface first.

Okay i understand but it can be fixed since getRoutesOptions() are implemented on the RouteCollection class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants