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

Added option to have empty paths #233

Merged
merged 4 commits into from Sep 17, 2014
Merged

Added option to have empty paths #233

merged 4 commits into from Sep 17, 2014

Conversation

gabrielbull
Copy link
Contributor

For example, this code:

$klein->with('/users/', function () use ($klein) {
    $klein->respond('GET', '', function () {
        echo 'Hello World';
    });
});

Responds to the given the path /users/, as it should, but throws an error:

Notice: Uninitialized string offset: 0 in /klein/klein/src/Klein/RouteFactory.php on line 84

This PR fixes that bug and allows for paths to be empty.
Please note that providing null as an argument for the path creates a catch-all route so this is not an option.

@Rican7
Copy link
Member

Rican7 commented Sep 16, 2014

Couldn't you achieve this same thing with just:

$klein->with('/users', function () use ($klein) {
    $klein->respond('GET', '/?', function () {
        echo 'Hello World';
    });
});

@gabrielbull
Copy link
Contributor Author

Yes

@Rican7
Copy link
Member

Rican7 commented Sep 16, 2014

Hmm, I can see a case for an empty string, but its implicit nature is strange to me.
It would need to be documented well.

@Rican7 Rican7 added the Feature label Sep 16, 2014
@Rican7 Rican7 self-assigned this Sep 16, 2014
@gabrielbull
Copy link
Contributor Author

Could it be replaced by a . like some other path system use to reference the current path (E.G. href attribute in HTML).

Example:

$klein->with('/users/', function () use ($klein) {
    $klein->respond('GET', '.', function () {
        echo 'Hello World';
    });
});

Should match the path /users/,that could be a feature.

But, I beleive my current pull request should be considered a bug fix since all it does is prevent an undesirable notice. Maybe we could remove the unit test as it may not be a documented behaviour, but it should not throw notices anyhow.

@Rican7
Copy link
Member

Rican7 commented Sep 16, 2014

Very true. Not checking for the offset first is bad, so this PR fixes that. An empty string is weird, but I guess its technically legal.

If you could just split up the line into a multi-line if block so it doesn't go beyond 120 characters, then the Travis style tests would pass. Maybe like this?:

if ($this->namespace
    && (isset($path[0]) && $path[0] === '@')
    || (isset($path[0]) && $path[0] === '!' && isset($path[1]) && $path[1] === '@')) {

@gabrielbull
Copy link
Contributor Author

Is that the intended condition? Because before it was this:

$this->namespace && $path[0] === '@' || ($path[0] === '!' && $path[1] === '@')

which match $this->namespace && $path[0] === '@' OR $path[0] === '!' && $path[1] === '@'

With the one you provided, it would be $this->namespace AND ($path[0] === '@' OR $path[0] === '!' && $path[1] === '@')

@Rican7
Copy link
Member

Rican7 commented Sep 16, 2014

The intended condition check was admittedly poorly written before.
That if block is actually not how it would be evaluated, as not wrapping it still gives this result:

screen shot 2014-09-16 at 4 44 53 pm

@gabrielbull
Copy link
Contributor Author

Alright, so, using your condition, this PR should be good to go. I'll open an issue to discuss the addition of the . string in paths.

@@ -53,7 +53,7 @@ public function testBuildBasic(
$should_match = true
) {
// Test data
$test_path = $test_path ?: '/test';
$test_path = !is_string($test_path) ? '/test' : $test_path;
Copy link
Member

Choose a reason for hiding this comment

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

I hate to be a pain, but this would be a bit more readable to me if it wasn't negated in front:

$test_path = is_string($test_path) ? $test_path : '/test';

@gabrielbull
Copy link
Contributor Author

Alright, done

@Rican7 Rican7 added the Bug label Sep 17, 2014
@Rican7
Copy link
Member

Rican7 commented Sep 17, 2014

Awesome! Tests passed!
Thank you so much for the PR, @gabrielbull. :)

Rican7 added a commit that referenced this pull request Sep 17, 2014
@Rican7 Rican7 merged commit 46e8916 into klein:master Sep 17, 2014
@Rican7 Rican7 added this to the 2.1 milestone Sep 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants