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: [Router] extract a class for auto-routing #5877

Merged
merged 18 commits into from
Apr 20, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Apr 8, 2022

Description

I want to introduce a new, more secure auto-routing.
In this PR, I extract the AutoRouter class from the Router class in order to be able to easily replace the functionality.

Thoughts:

  • I wanted to avoid instantiating AutoRouter when auto routing is off. But the current implementation mixes the processing of defined routes and auto routes. So it was impossible.
  • It was difficult to understand the code because there were many reassignments to variables and changes of properties by setters.

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 the refactor Pull requests that refactor code label Apr 8, 2022
@kenjis kenjis mentioned this pull request Apr 9, 2022
5 tasks
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I like this conceptually. This PR is larger than what I want to review on mobile, but it looks like mostly just move methods to the new class.

I think this is overall a "simplification", in the it distinguishes two features and leaves each in its own class, but @lonnieezell might want to weigh in as well.

system/Router/AutoRouter.php Outdated Show resolved Hide resolved
system/Router/Router.php Outdated Show resolved Hide resolved
system/Router/AutoRouter.php Show resolved Hide resolved
system/Router/AutoRouter.php Show resolved Hide resolved
system/Router/AutoRouter.php Show resolved Hide resolved
system/Router/AutoRouter.php Outdated Show resolved Hide resolved
system/Router/Router.php Show resolved Hide resolved
system/Router/AutoRouter.php Show resolved Hide resolved
system/Router/Router.php Outdated Show resolved Hide resolved
system/Router/AutoRouter.php Outdated Show resolved Hide resolved
system/Router/AutoRouter.php Outdated Show resolved Hide resolved
@kenjis kenjis force-pushed the refactor-router branch 2 times, most recently from e302a3d to ddb816b Compare April 11, 2022 07:48
@kenjis
Copy link
Member Author

kenjis commented Apr 11, 2022

Does anybody know why these tests fail?

1) CodeIgniter\Database\Migrations\MigrationRunnerTest::testLatestTriggersEvent
Failed asserting that null is of type "array".

/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/Migrations/MigrationRunnerTest.php:355
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:97

2) CodeIgniter\Database\Migrations\MigrationRunnerTest::testRegressTriggersEvent
Failed asserting that null is of type "array".

/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/Migrations/MigrationRunnerTest.php:374
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:97

@kenjis
Copy link
Member Author

kenjis commented Apr 12, 2022

I found the root cause: 5e5f3fc

@kenjis kenjis marked this pull request as draft April 12, 2022 02:29
@kenjis kenjis marked this pull request as ready for review April 12, 2022 02:53
@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities labels Apr 15, 2022
@kenjis
Copy link
Member Author

kenjis commented Apr 19, 2022

I would like to merge this PR.

Maintainers, please at least comment on the following parts of the PR:

Reviews are of course welcome.

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I have no preference with naming methods. As long as it is obvious what a method does, then it's fine with me. Minor comments only on the bug fix.

system/Router/AutoRouter.php Outdated Show resolved Hide resolved
system/Router/AutoRouter.php Outdated Show resolved Hide resolved
kenjis and others added 4 commits April 20, 2022 14:55
In setUp() Event simulate is set to true, so it should be restored in tearDown().
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
@kenjis kenjis merged commit 1fe143b into codeigniter4:develop Apr 20, 2022
@kenjis kenjis deleted the refactor-router branch April 20, 2022 08:18
@kenjis
Copy link
Member Author

kenjis commented Apr 20, 2022

Thank you, @paulbalandan !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: $routes->cli() accessible via web browser if autoroute is true
4 participants