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(App): Remove registerRoutes method #42678

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Jan 10, 2024

Summary

Removes the deprecated App::registerRoutes method.
The RouteConfig class is no longer used so I deleted it as well including it's tests.
For some reason much of the implementation was the same as RouteParser, but that one has no tests 🤔.

Checklist

Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
Copy link
Member Author

OpenAPI is failing because it previously failed to discover all the routes in user_ldap so they are missing now and need to be documented.

use OC\Core\Application;
/** @var $this OCP\Route\IRouter */
// Core ajax actions
$this->create('core_ajax_update', '/core/ajax/update.php')

Check notice

Code scanning / Psalm

InvalidScope Note

Invalid reference to $this in a non-class context
/** @var string[] */
private $controllerNameCache = [];

protected $rootUrlApps = [
Copy link
Member

Choose a reason for hiding this comment

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

???

How is this implemented now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Check RouteParser, it has exactly the same apps listed and the same logic 🤷‍♀️. Think someone just copied parts of the implementation which lead to this logic duplication.

Copy link
Member

Choose a reason for hiding this comment

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

🙈

@nickvergessen
Copy link
Member

The RouteConfig class is no longer used so I deleted it as well including it's tests.
For some reason much of the implementation was the same as RouteParser, but that one has no tests 🤔.

Time to migrate the tests over then?

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jan 10, 2024
@provokateurin
Copy link
Member Author

Time to migrate the tests over then?

Yes, but I took a look and I'm not really sure what the tests are trying to achieve. Are they testing that the routes are correctly parsed or are they checking that the routes are added correctly to the Router?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants