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

Implementing reverse routing syntax #11688

Merged
merged 4 commits into from Feb 7, 2018

Conversation

Projects
None yet
5 participants
@burzum
Member

burzum commented Feb 2, 2018

See these two links

@burzum burzum requested a review from markstory Feb 2, 2018

static::$initialized = true;
static::scope('/', function ($routes) use ($route, $defaults, $options) {
$routes->connect($route, $defaults, $options);
});
}
/**

This comment has been minimized.

@stickler-ci

stickler-ci Feb 2, 2018

Contributor

Doc comment for parameter "$defaults" missing

/**
* Parse the defaults if they're a string
*
* @param string|array

This comment has been minimized.

@stickler-ci

stickler-ci Feb 2, 2018

Contributor

Missing parameter name

@@ -703,8 +703,12 @@ public function loadPlugin($name, $file = 'routes.php')
* @throws \InvalidArgumentException
* @throws \BadMethodCallException
*/
public function connect($route, array $defaults = [], array $options = [])
public function connect($route, array $defaults = null, array $options = [])

This comment has been minimized.

@ADmad

ADmad Feb 3, 2018

Member

Why change the default value?

@markstory markstory added this to the 3.6.0 milestone Feb 4, 2018

'prefix' => $matches[2],
'controller' => $matches[3],
'view' => $matches[4]
];

This comment has been minimized.

@markstory

markstory Feb 4, 2018

Member

There should probably be an exception thrown for the default case.

];
$this->assertEquals($result, $expected);
Router::connect('/admin/blog/articles/view', 'Blog.Admin\Articles::view');

This comment has been minimized.

@markstory

markstory Feb 4, 2018

Member

What about nested prefixes? Those have caused us troubles in other places.

This comment has been minimized.

@burzum

burzum Feb 4, 2018

Member

Honestly, I've never used nested prefixed. Guess we use everything in front of the first match as "path" for the nested prefix?

This comment has been minimized.

@markstory

markstory Feb 5, 2018

Member

Or all tokens separated by \ except the last one are the prefix, and the last one is the controller name.

This comment has been minimized.

@burzum

burzum Feb 5, 2018

Member

@markstory like that?

image

How does it has to end up in the matches? With this regex it would be one string. We could simply use explode() to break it up if it contains \.

This comment has been minimized.

@markstory

markstory Feb 5, 2018

Member

Is Foo\Bar\Admin the prefix? Or the plugin name there?

This comment has been minimized.

@burzum

burzum Feb 5, 2018

Member

Prefix? Just specific exactly how you would like to have it implemented? :) Can you just commit a test case for the syntax you want and I'll take care of the rest.

@codecov-io

This comment has been minimized.

codecov-io commented Feb 4, 2018

Codecov Report

Merging #11688 into 3.next will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             3.next   #11688      +/-   ##
============================================
+ Coverage     92.11%   92.14%   +0.03%     
- Complexity    13242    13252      +10     
============================================
  Files           462      462              
  Lines         34005    34201     +196     
============================================
+ Hits          31324    31516     +192     
- Misses         2681     2685       +4
Impacted Files Coverage Δ Complexity Δ
src/Routing/Router.php 98.67% <ø> (ø) 118 <0> (ø) ⬇️
src/Routing/RouteBuilder.php 99.61% <100%> (+0.04%) 96 <17> (+9) ⬆️
src/Utility/Hash.php 97.68% <0%> (-0.01%) 247% <0%> (+1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e7bdbc...74ee091. Read the comment docs.

/**
* Parse the defaults if they're a string
*
* @param string|array Defaults array from the connect() method.

This comment has been minimized.

@stickler-ci

stickler-ci Feb 4, 2018

Contributor

Missing parameter name

Move string target parsing into RouteBuilder.
Having it in RouteBuilder makes it accessible to the HTTP method
helpers, as well as connect() on the scoped builder.

Prefix names need to be lower cased, so that has been updated as well.

I've added tests for nested plugins and nested prefixes as well.
@markstory

This comment has been minimized.

Member

markstory commented Feb 6, 2018

@burzum I've done the changes I think needed to be done and pushed them up.

@markstory markstory referenced this pull request Feb 6, 2018

Closed

Improve ergonomics of defining routes #10689

6 of 6 tasks complete
@burzum

This comment has been minimized.

Member

burzum commented Feb 6, 2018

@markstory looks good.

@markstory markstory self-assigned this Feb 7, 2018

@markstory

This comment has been minimized.

Member

markstory commented Feb 7, 2018

I will merge this once I get the documentation updates done.

markstory added a commit to cakephp/docs that referenced this pull request Feb 7, 2018

@markstory markstory merged commit e09ff9b into 3.next Feb 7, 2018

5 checks passed

codecov/patch 100% of diff hit (target 92.11%)
Details
codecov/project 92.14% (+0.03%) compared to 9e7bdbc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
stickler-ci No lint errors found.

@markstory markstory deleted the 3.next-reverse-routing-syntax branch Feb 7, 2018

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