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

Version numbering #16

Closed
sestolk opened this issue Apr 28, 2014 · 6 comments
Closed

Version numbering #16

sestolk opened this issue Apr 28, 2014 · 6 comments

Comments

@sestolk
Copy link

sestolk commented Apr 28, 2014

I seem to be running into a problem of which I dont know if I'm doing something wrong or it is some sort of restriction.

I am currently testing your API to see if it fulfills the requirements for a project I will be working on. For this project I would like version numbers that use 3 digits (length), for example: 0.0.1, 1.0.3 etc.

When I create the API groups with simple digits v1, v2, v3 it works fine, but when I start to use v1.1.0 (also tried only 2 digits) or v110 it will always default back to v1.

So this works:

Route::api(['version' => 'v1', 'namespace' => 'v001'], function()
{ ... });
Route::api(['version' => 'v2', 'namespace' => 'v002'], function()
{ ... });
Route::api(['version' => 'v3', 'namespace' => 'v003'], function()
{ ... });

But when I change it to:

Route::api(['version' => 'v1', 'namespace' => 'v001'], function()
{ ... });
// v11 or v1.1 or v1.1.0 tried several combinations
Route::api(['version' => 'v11', 'namespace' => 'v011'], function()
{ ... });
Route::api(['version' => 'v3', 'namespace' => 'v003'], function()
{ ... });

It will always default back to v1 as if the version is not properly translated.

I used Postman to test the endpoints with the following Accept header:

application/vnd.test.v1.1+json
// or
application/vnd.test.v11+json
// or
application/vnd.test.v1.1.0+json

'test' is configured in the config.php

Could you tell me what I am doing wrong? If anything is unclear, please let me know!

@jasonlewis
Copy link
Contributor

Hm, to be honest I never gave point release API versions a though. I'll push a fix for this shortly.

@sestolk
Copy link
Author

sestolk commented Apr 28, 2014

I may have found your preg_match that checks the version number. Trying to get it working with my examples. Thanks for the quick reply, looking forward to your fix!

@jasonlewis
Copy link
Contributor

Yeah the ApiRouteCollection should actually check itself if the request matches its version. Currently it doesn't.

Just need to write some tests for this and I'll have it pushed and tagged.

@sestolk
Copy link
Author

sestolk commented Apr 28, 2014

Ok, I've got it working by changing one line of code.

I changed line 430 in Routing\Router.php to:

if (preg_match('#application/vnd\.'.$this->vendor.'.(v[0-9\.]{1,6})\+(json)#', $request->header('accept'), $matches))

I think it is quite obvious what it does, but it now matches every pattern untill 6 characters. So even 10.0.1 is possible. Maybe add this regex to the config?

I believe its best that you apply my fix (If its good enough ofcourse) since im not that great at using Github.

@jasonlewis
Copy link
Contributor

There was actually more to it then just the point release API versions. There was an issue with the router prematurely matching a collection when the collection may not have actually contained the route that the request was initially targeting. I've tagged v0.1.1 so you can now composer update.

@sestolk
Copy link
Author

sestolk commented Apr 28, 2014

Sweet, thanks for the quick fix! Waiting for the package to populate the servers.

wayne5w referenced this issue Mar 17, 2016
Instead of using the existing Laravel router we will use a new instance. This
should prevent the APIs internal dispatcher from overriding the current route
on the actual Laravel router instance.

There does not seem to be any need for the router instance that is injected
into the adapter to be the actual Laravel router, as the adapter just needs
an instance to dispatch the API-only routes.
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

2 participants