Skip to content

fix($urlMatcherFactory): early binding of array handler bypasses type resolution #1572

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

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 20, 2014

Is this (one of) the causes of issue #1048 ?

Please do NOT pull just yet as there are follow-up bugs which I didn't fix yet. Just wanted feedback whether I'm going in the right direction.

@christopherthielen
Copy link
Contributor

Nice catch. I'm actually working on this code right now.

This is how I modified it:

    this.encode = arrayHandler(bindTo(type, type.encode));
    this.decode = arrayHandler(bindTo(type, type.decode));
    this.is     = arrayHandler(bindTo(type, type.is), true);
    this.equals = arrayEqualsHandler(bindTo(type, type.equals));
    this.pattern = type.pattern;
    this.$arrayMode = mode;

@ghost
Copy link
Author

ghost commented Nov 20, 2014

Hi Christoph, my patch actually is against master not one of the releases, so I think I got your change already. The code you posted above still seems to be affected as it binds the functions rather than keeping the object in the closure. (Sorry, I had to edit the text because I couldn't finish due to a "forced" software update.)

@christopherthielen
Copy link
Contributor

@Jerico-Dev I'm not convinced that's necessary. However, don't spend too much time on it, as I've made significant changes to that area of the code that I haven't yet committed. I plan to commit later tonight.

@christopherthielen
Copy link
Contributor

@Jerico-Dev I looked at your test and now see what you mean. Thank you!

@ghost
Copy link
Author

ghost commented Nov 20, 2014

For now I desisted using custom types as I had difficulties in formatting URLs (the custom type API was called sometimes with encoded values on encode() and sometimes with the decoded values on decode() and both on is(). Guess you're straightening this out in 0.2.13, so I'll hold on. If you like you can pull the fix then...

@christopherthielen
Copy link
Contributor

FYI, .encode() gets called with encoded values. It is sorta like a .normalize() fn. However, .decode() should not be called with pre-decoded value.

@christopherthielen
Copy link
Contributor

merged in via e9f8b8a

@ghost
Copy link
Author

ghost commented Nov 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant