search

basz / SlmLocale

Auto detection of locale through different strategies for Zend Framework 2
Other
68 stars 34 forks source link

Refactoring #25

Closed juriansluiman closed 11 years ago

juriansluiman commented 11 years ago

@basz I am refactoring SlmLocale now. If you'd like, please review :) This is still WIP.

The need for refactoring is for a couple of reasons:

  1. When a locale is detected, the strategy did a send() on the Response. Now the strategy returns a modified response, so all strategies can change the response to perform one redirect.
  2. Removal of global aliases, as ".com" is the alias for the TLD strategy but "en" is the alias for the uri strategy, with the exact same locale.
  3. Implementing the current PRs for view helpers on top of the refactored code base
basz commented 11 years ago

@juriansluiman all makes sense to me

juriansluiman commented 11 years ago

@basz I finally had time to finish the tests for this refactoring. All is green, for now. I commented out three tests which I did not understand:

  1. What is this feature supposed to do? You set it explicitly to not redirect but is performs a redirect?
  2. Redirecting a base url without trailing slash apparently performed the redirect towards a trailing slash. Not sure it is a desired feature, but I have not implemented this in the refactoring.

About that last point: if you have a route "en" en you have the language "en", it becomes the route "en/en". If you perform the above redirect and start at "en", then you're redirected to "en/" so the "/en" is made "/". I can't imagine it will happen often, but is is a drawback of automated redirection. Thoughts?

juriansluiman commented 11 years ago

Merging now, we can reimplement above features at any time (v0.0.1 is still featuring the old code).

basz commented 11 years ago

I think (it's been a while) I wanted to make sure that when an locale is detected in the first segment of an path, behavior was similar to that of apache encountering a directory regardless of the redirect option. http://blah.com/something is http://redirected to blah.com/something/

Basicly an insurance against developers forgetting the trailing slash and preventing a 404. But when I think about it now it feels a bit overly concerned and it is introducing magic which we can do without.

It would require additional logic at https://github.com/juriansluiman/SlmLocale/blob/94d5496e9631d2f59e5e584257a6a20d5cf71fb5/src/SlmLocale/Strategy/UriPathStrategy.php#L169 or something similar what I had at https://github.com/juriansluiman/SlmLocale/commit/bed19e43e565a124709ad7e022970ecac4ec3e83#L0R121

But honestly, perhaps it's better to take a step back and remove that...