search

FriendsOfSymfony / FOSJsRoutingBundle

A pretty nice way to expose your Symfony routing to client applications.
1.48k stars 261 forks source link

Fix regression introduced by fix for Symfony 3, check for undefined schemes key #288

Closed tobias-93 closed 7 years ago

tobias-93 commented 7 years ago

Fix for #287, problem introduced by #254

GuilhemN commented 7 years ago

Any Idea why the tests are failing?

tobias-93 commented 7 years ago

@GuilhemN I think the tests are a little more strict than browsers are. I just added another check which should not trigger the test to fail.

tobias-93 commented 7 years ago

Ok, I'll have to look into them, as they still fail

willdurand commented 7 years ago

I see a lot of:

TypeError: 'undefined' is not an object (evaluating 'route.schemes.length') 'undefined' is not an object (evaluating 'route.schemes.length') TypeError: 'undefined' is not an object (evaluating 'route.schemes.length')
maguedon commented 7 years ago

I think it is because the property "schemes" may not exist in some cases. You should check if it exists first. Why did you remove the condition goog.object.containsKey(route, "schemes") ? I suppose it checks if "schemes" exists. Does it work if you add it at the beginning of your condition like this :

else if (goog.object.containsKey(route, "schemes") && route.schemes.length > 0 && typeof route.schemes[0] !== "undefined" && this.getScheme() != route.schemes[0]) {
tobias-93 commented 7 years ago

I forgot to add the new object keys to the JS tests, so it should be fixed now

tobias-93 commented 7 years ago

The tests now work again. @maguedon that would also fix it, but we can assume this key will always exist since the Symfony command always adds it to the object.

willdurand commented 7 years ago

The tests now work again. @maguedon that would also fix it, but we can assume this key will always exist since the Symfony command always adds it to the object.

Is that 100% sure? For BC purpose I would rather go for a test and not assume it exists..

tobias-93 commented 7 years ago

@willdurand It should be. I added the check anyway, just to be sure.

GuilhemN commented 7 years ago

Is this ok to merge @willdurand ?

willdurand commented 7 years ago

It would be better not to update the whole test suite to ensure BC but otherwise sure :+1:

tobias-93 commented 7 years ago

@willdurand I have removed the updates to the test suite, so just the new tests remain. @GuilhemN I have removed the duplicate check. Is this ready to be merged now?

willdurand commented 7 years ago

Looks good to me!