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

add basepath support #96

Merged
merged 3 commits into from
Jul 17, 2015
Merged

add basepath support #96

merged 3 commits into from
Jul 17, 2015

Conversation

pmjones
Copy link
Member

@pmjones pmjones commented Jul 16, 2015

This should be a fix for #86 -- @harikt @pine3ree please review and comment.

Basically, you create a RouterFactory with the basepath you want; that would be the subdirectory in which the application lives. You can then add routes as normal, without having to specify the basepath prefix; the matcher will match properly, and the generator will generate paths with the basepath prefix in place.

$this->assertSame('DirectAction', $actual->values['action']);

// should get the basepath in place
$expect = '/path/to/sub/index.php/foo/bar';
Copy link
Member

Choose a reason for hiding this comment

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

good to have one more test with no index.php . So is /path/to/sub/foo/bar

@harikt
Copy link
Member

harikt commented Jul 17, 2015

Thank you @pmjones . This looks good to me.

@pine3ree
Copy link

Thank You @pmjones
Looks fine to me.
To be on the safe side i would add

//...
$this->basepath = rtrim($basepath, ' /');

to the Router constructor for handling params like '/path/to/sub/'

@pmjones
Copy link
Member Author

pmjones commented Jul 17, 2015

Added a dir-only test, and added rtrim. When the CI passes, I'll merge it. Thanks guys!

pmjones pushed a commit that referenced this pull request Jul 17, 2015
@pmjones pmjones merged commit a343526 into 2.x Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants