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

Implemented bracketed optional parameters #35

Closed
wants to merge 3 commits into from
Closed

Implemented bracketed optional parameters #35

wants to merge 3 commits into from

Conversation

adrianmiu
Copy link

This allows one to create routes like

  • [/{language}]/pages[.{type}] which will match /pages and /en/pages and /en/pages.json
  • posts/archive[/{year}][/{month}][/{day}]

The generate() method makes sure that parameters that have the default value are not included. So, if the language default value is en the URL that will be generated will NOT start with /en

@adrianmiu adrianmiu mentioned this pull request Jan 25, 2014
@adrianmiu
Copy link
Author

BTW, the code coverage remains 100%

@harikt
Copy link
Member

harikt commented Jan 25, 2014

@adrianmiu Thank you.

@pmjones will check and respond.

$this->assertTrue($foo->isMatch('/blog/archive/2014/01', array()));

$this->assertEquals(
[
Copy link
Member

Choose a reason for hiding this comment

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

short array, will not get passed in 5.3 .

@harikt
Copy link
Member

harikt commented Jan 30, 2014

So I have talked with @pmjones regarding this.

Please have a look into the discussion logs at http://colabti.org/irclogger/irclogger_log/auraphp?date=2014-01-30#l53 .

Thanks

@adrianmiu
Copy link
Author

From the discussion it seems the problem is the array short notation from the $this->assertEquals(... which can easily be changed.
Otherwise I don't see the problem with having a second notation. ZF2 also uses the brackets to denote optional parameters (though they use :parameter_name to denote required parameters). I think that having {/language} is wrong for 3 reasons:

  1. it introduces additional processing for handlebars segments (both in processing a route and generating a URL)
  2. it has reduced readability. [/{language}] vs {/language}
  3. it is more flexibile. You can have [_x_{language}] but you can't have {_x_language} (is this a required param or an optional one?)

@pmjones
Copy link
Member

pmjones commented Jan 30, 2014

So, the issue of notation stems from the (admittedly non-accepted) RFC on URI templates. This comes from the fact that we already have an "optional" notation in the form of {/month,day,year} (note the slash inside the braces).

If we expand on that idea, the "required" notation for a required param would be {foo}, "optional" for a slash-prefixed param would be {/foo}, and "optional" for a dot-prefixed param would be {.foo}.

Whether that's the right thing to do or not is still a matter for discussion, but I do think it bears discussing.

Update: your point regarding "You can have[_x_{language}] but you can't have {_x_language}" is well-taken.

@pmjones
Copy link
Member

pmjones commented Jan 30, 2014

Tell you what @adrianmiu -- fix the short-array stuff so it passes on 5.3, and I can merge it manually to play around with it. Then we can follow up after that. (FWIW I do see the value of the patch.)

@adrianmiu
Copy link
Author

I've made the required changes for 5.3.
I've just finished looking (superficially mind you) at the URI templates RFC and the first thing that struck me is that the RFC would require the routes to be defined like

{/controller}{/action}{/id}{.type}

which seem that are not very useful for optional parameters like the language. Except thigns like {/path} and {/param,param2,param3} I haven't found anything in the RFC that is helpful for solving the "optional parameters" problem.
Or I haven't properly understood the RFC :)

I do like the fact that the / is included in the parameters because, potentially and with some additional work, one can do a route like so /page{/slug}{-id}{.type} (eg: /page/contact-123.html).

@adrianmiu
Copy link
Author

I think it's worth repeating one more thing related to the optional parameters that I've implemented in my code: optional parameters that have the default value are not included when generating the URL. This is very useful for SEO purposes since it will let you make a 301 redirect from /en/controller/action/id to /controller/action/id if the default language is en.

I can't think of a solution to do that if you follow the RFC, even you find a way to use the RFC for matching.

@harikt
Copy link
Member

harikt commented Aug 25, 2014

@pmjones this is a long time PR which needs you .

@pmjones
Copy link
Member

pmjones commented Aug 25, 2014

I'm sorry, man, but after letting this percolate it just doesn't sit well with me, at least not for now.

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.

None yet

3 participants