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

Required parameters evaluating as optional #48

Closed
ramsey opened this issue May 10, 2012 · 2 comments
Closed

Required parameters evaluating as optional #48

ramsey opened this issue May 10, 2012 · 2 comments
Assignees

Comments

@ramsey
Copy link
Contributor

ramsey commented May 10, 2012

I created a route that looks like this:

/accounts/*/catalogs/*

According to the documentation, that first asterisk is not an optional parameter. However, requests to /accounts/catalogs is matching on that. Shouldn't it be giving me a 404 or something?

Reproduction script:

<?php

include '../vendor/autoload.php';

$r3 = new \Respect\Rest\Router();

$r3->get('/accounts/*/catalogs/*', function($accountId, $catalogId = null) {
    echo "Account ID: {$accountId}, Catalog ID: {$catalogId}";
});

Request:

GET /accounts/catalogs HTTP/1.1
Host: localhost:8181
Content-Type: application/x-www-form-urlencoded; charset=utf-8
Accept-Encoding: identity, deflate, compress, gzip
Accept: */*
User-Agent: HTTPie/0.1.7-dev

Response:

HTTP/1.1 200 OK
Date: Thu, 10 May 2012 17:39:41 GMT
Server: Apache/2.2.20 (Ubuntu)
X-Powered-By: PHP/5.4.0-3~oneiric+4
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 42
Content-Type: text/html

Account ID: , Catalog ID: 
@alganet
Copy link
Member

alganet commented May 10, 2012

Wow, it seems to be a bug. We had past issues with optional parameters, seems like they're back.

I'm assigning me and @augustohp to fix this. Matches by path are done by regular expressions on AbstractRoute, we'll review that logic.

@ghost ghost assigned alganet and augustohp May 10, 2012
@alganet
Copy link
Member

alganet commented May 17, 2012

@ramsey this should be working properly now! Thanks to this issue, we also simplified our pattern matching and possibly improved performance a lot. Thank you!

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

No branches or pull requests

3 participants