Skip to content

New syntax - #225

Open
rustamwin wants to merge 40 commits into
masterfrom
new-syntax
Open

New syntax#225
rustamwin wants to merge 40 commits into
masterfrom
new-syntax

Conversation

@rustamwin

@rustamwin rustamwin commented Nov 9, 2023

Copy link
Copy Markdown
Member
Q A
Is bugfix?
New feature? ✔️
Breaks BC? ✔️
Fixed issues #193, #171

Current syntax

- use Yiisoft\Router\Route;
- use Yiisoft\Router\Group;
+ use Yiisoft\Router\Builder\RouteBuilder as Route;
+ use Yiisoft\Router\Builder\GroupBuilder as Group;


Group::create()->routes(
    Route::get('')->name('index'),
-   Route::get('/posts')->name('posts'),
+   Route::get('/posts')->name('posts'), // No getters, only setters
);

@rustamwin rustamwin added the status:code review The pull request needs review. label Nov 9, 2023
@rustamwin
rustamwin requested a review from a team November 9, 2023 07:22
@codecov

codecov Bot commented Nov 9, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 349 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (e34cc7a) to head (2871cf8).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/Route.php 0.00% 107 Missing ⚠️
src/Builder/RouteBuilder.php 0.00% 73 Missing ⚠️
src/Group.php 0.00% 64 Missing ⚠️
src/Builder/GroupBuilder.php 0.00% 49 Missing ⚠️
src/RouteCollection.php 0.00% 40 Missing ⚠️
src/RouteCollector.php 0.00% 9 Missing ⚠️
src/CurrentRoute.php 0.00% 6 Missing ⚠️
src/Middleware/Router.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #225    +/-   ##
=========================================
  Coverage      0.00%   0.00%            
- Complexity      129     201    +72     
=========================================
  Files            13      15     +2     
  Lines           400     544   +144     
=========================================
- Misses          400     544   +144     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@what-the-diff

what-the-diff Bot commented Nov 9, 2023

Copy link
Copy Markdown

PR Summary

This pull request introduces numerous updates aimed at better organizing and streamlining how routes are handled in the system:

  • New Classes

    • Two new classes, GroupBuilder and RouteBuilder, have been added. These classes represent a group of routes and a single route respectively, and they include methods for defining diverse route characteristics, such as HTTP methods, route pattern, middlewares, hosts, and more.
  • Method and Property Modifications

    • Several existing methods (getName(), getHost(), getPattern(), getMethods()) in the CurrentRoute.php file and Debug/RouterCollector.php were adjusted to use new direct methods instead of a generic getData() method.
    • In the Group.php file, some properties were removed and replaced with new ones, and a variety of new methods designed for handling these properties were added.
  • “Route”

    • The Route.php file underwent a major update where new properties were added, methods were adjusted to utilize these new properties, and unnecessary methods were removed. The changes aim to streamline the usage of the class and make it more efficient.
  • Route Collection Update

    • The RouteCollection.php was refactored to better use new methods in the Route class and to remove outdated code.
  • Route Collector Update

    • The RouteCollector.php underwent several property and method name changes and had type assertions and argument validations added to improve code robustness and reliability.
  • Inclusion of Test Cases

    • A new test file for RouteBuilder class was added and several existing test files were updated to comply with the changes made to the core classes.
  • Miscellaneous

    • Adjustments were made in RouteCollectionTest.php and RouteCollectorTest.php to reflect changes made in the main classes.

These changes all contribute to a more robust and efficient route handling system within the software, aligning the codebase with modern best practices in PHP coding.

@vjik vjik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. #191 suggest use named arguments, and this must improve performance. In this PR keep routes/groups object and additional add new routes/groups objects with named arguments. Seems, this decrease performance...

  2. This not fix #171.

@rustamwin

Copy link
Copy Markdown
Member Author

In this PR keep routes/groups object and additional add new routes/groups objects with named arguments. Seems, this decrease performance...

We can mention it in docs or drop the current syntax.

This not fix #171.

Yes, doesn't fix directly. Since there is no exception, there is no need for it.

@vjik

vjik commented Nov 22, 2023

Copy link
Copy Markdown
Member

We can mention it in docs or drop the current syntax.

But what goal of syntax change?

# Conflicts:
#	src/Debug/RouterCollector.php
#	src/Route.php
#	tests/RouteTest.php

@vjik vjik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we make Route and Group constructors public and keep BC?

I think, GroupBuilder and RouteBuilder are not needed.

@vjik vjik mentioned this pull request Nov 2, 2025
@rustamwin

Copy link
Copy Markdown
Member Author

Can we make Route and Group constructors public

Did you mean builders? Route and Group constructors are public in this PR.

I think, GroupBuilder and RouteBuilder are not needed.

Will we abandon the current syntax?

@vjik

vjik commented Nov 7, 2025

Copy link
Copy Markdown
Member

Can we make Route and Group constructors public

Did you mean builders? Route and Group constructors are public in this PR.

No, I mean Yiisoft\Router\Router and Yiisoft\Router\Group

I think, GroupBuilder and RouteBuilder are not needed.

Will we abandon the current syntax?

Mark as deprecated, then remove in major version.

@rustamwin

Copy link
Copy Markdown
Member Author

Mark as deprecated, then remove in major version.

Let's abandon right away since the PR has major changes.

@vjik

vjik commented Nov 13, 2025

Copy link
Copy Markdown
Member

Let's abandon right away since the PR has major changes.

What major changes are needed?

# Conflicts:
#	composer.json
#	src/Debug/DebugRoutesCommand.php
#	src/Route.php
…on, and apply `readonly` for constructor injection
…dlewares and adjust defaults assignment logic

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/RouteCollection.php
Comment thread src/Debug/RouterCollector.php Outdated
Comment thread src/Route.php Outdated
Comment thread src/CurrentRoute.php
@samdark

samdark commented Jul 24, 2026

Copy link
Copy Markdown
Member

@rustamwin resolving of conflicts is needed.

Comment thread README.md Outdated
@samdark

samdark commented Jul 29, 2026

Copy link
Copy Markdown
Member

I also think we can keep backwards compatibility and mark some methods as deprecated but not sure it's needed.

@samdark
samdark requested review from vjik and a lite review from Copilot August 5, 2026 11:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/Debug/RouterCollector.php:92

  • Same as above: hosts currently returns an imploded string. Prefer returning the host list for a clearer debug data shape.
            'hosts' => implode(', ', $route->getHosts()),

src/Builder/GroupBuilder.php:59

  • routes() assigns the variadic $routes array directly, which preserves named-argument keys (e.g. routes(a: $r1, b: $r2)) and can yield a non-list array in the resulting Group. Normalize to a list with array_values() to keep ordering predictable and consistent with other variadic APIs here (e.g. middleware()/prependMiddleware() use array_values()).
    public function routes(Group|Route|RoutableInterface ...$routes): self
    {
        $new = clone $this;
        $new->routes = $routes;

        return $new;

src/Group.php:121

  • setRoutes() stores the provided array as-is. If callers pass an associative array (or if a builder/variadic call preserves named-argument keys), getRoutes() will no longer be a list and indexing like $group->getRoutes()[0] can break. Consider normalizing with array_values() when assigning.
    public function setRoutes(array $routes): self
    {
        $this->assertRoutes($routes);
        $this->routes = $routes;
        return $this;

src/Debug/RouterCollector.php:59

  • The currentRoute.hosts field name is plural, but the value is a comma-separated string. Returning the actual host list makes the collected data less ambiguous (and avoids '' vs null when there are no hosts).

This issue also appears on line 92 of the same file.

                'hosts' => implode(', ', $route->getHosts()),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants