search

symfony-cmf / create-bundle

UNMAINTAINED - Integrates create.js and createphp into Symfony2
http://cmf.symfony.com/
29 stars 31 forks source link

Remove locale prefix from rest routes #126

Closed EmmanuelVella closed 10 years ago

EmmanuelVella commented 10 years ago
Q A
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Doc PR To come

I think the _locale prefix should (or not) be set globally when importing the rest.xml file.

I personnaly have one domain per locale so I don't need to prefix these routes.

If this PR is accepted, I will add the corresponding documentation.

BTW, I submit this PR against the 1.1 branch, hope it's ok !

lsmith77 commented 10 years ago

i think at this point we cannot change it but we could offer a second routing xml file without locale ..?

EmmanuelVella commented 10 years ago

Yes, or I can submit this PR against the master branch if you prefer.

dbu commented 10 years ago

i agree that we should not break this for existing versions. duplicating the file is a bit unlucky - would it work to provide {_locale} as prefix? then we could just document to include the file like that for multilang setups that use the locale from the path. we should probably change the file name at the same time, so that people see they need to check the changelog.

then again, this is just for rest calls right? sending the locale in the url again is a rather "hidden" issue and should have no potential for bugs.

EmmanuelVella commented 10 years ago

Yes, this is just for rest calls.

My problem is that I just did a migration from locale prefixed routes (/{_locale}) to localized domains, and I now redirect every URI starting with /{_locale} to the corresponding domain. So now when calling the rest URLs to save content, it's also being redirected, breaking the edition.

I know I could add an "exception" in my redirections rules, but I think it's anyway cleaner to let the user choose if he wants to prefix or not these routes.

And yes prefixing the import with /{_locale} works. I can add a note in the changelog and eventually rename the file if this solution is ok for you.

lsmith77 commented 10 years ago

what I meant was .. your change is ok .. but do it in a file rest_no_locale.xml, and then change the existing one to load that one with a prefix for BC

EmmanuelVella commented 10 years ago

@lsmith77 I didn't know that was possible, that's a great idea. I just updated my PR. Thanks !

lsmith77 commented 10 years ago

looks good to me .. did someone test this in the sandbox? we can then merge it and tag 1.2.1

EmmanuelVella commented 10 years ago

I tested it in my project by including the deprecated rest.xml file, it worked perfectly. Please notice that this PR is submitted on the 1.1 branch.

lsmith77 commented 10 years ago

we will not do more releases on 1.1, so I will merge it into master then.

EmmanuelVella commented 10 years ago

Ok, do you want me to create another PR on the master branch ?

lsmith77 commented 10 years ago

seems like there are some merge conflicts .. can you rebase and create a new PR for master?

EmmanuelVella commented 10 years ago

I opened a new PR here https://github.com/symfony-cmf/CreateBundle/pull/127