Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

encode uri: Matrix urls, semicolons fix 4065 #4067

Closed
wants to merge 1 commit into from
Closed

encode uri: Matrix urls, semicolons fix 4065 #4067

wants to merge 1 commit into from

Conversation

pahlers
Copy link

@pahlers pahlers commented Sep 20, 2013

Solved issue 4065, Support for Matrix URIs
#4065

  • added ; to the list of replacements
  • added a unit test

- added ; to the list of replacements
- added a unit test
@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@pahlers
Copy link
Author

pahlers commented Sep 20, 2013

Can you translate the CLA to Dutch?

@petebacondarwin
Copy link
Member

Basically, what you are saying is that the path of a URI is made up of segments, which are defined as follows::

segment       = *pchar
pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
sub-delims  = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="

Currently, encodeURIComponent() does not encode !, *, ', ( or ) and we have special cases for &, =, +, @, :, $ and , but you are suggesting we include ; as a special case, too.

I agree with this but if you look at the code, it seems to me that we need to fix the encodeUriQuery() function too and also that encodeUriSegment() should not be relying on encodeUriQuery() since it is actually not a super set - a query can include a ? which a segment cannot.

@ghost ghost assigned petebacondarwin Sep 20, 2013
@petebacondarwin
Copy link
Member

@eonlepapillon - are you able to sign the CLA if it is not translated?

@pahlers
Copy link
Author

pahlers commented Sep 21, 2013

Of course I can sign the CLA. The CLA would be more understandable if it was written in one's own language.
This lawyers language reads like Chinese for me.

@pahlers pahlers closed this Sep 21, 2013
@pahlers pahlers reopened this Sep 21, 2013
@pahlers
Copy link
Author

pahlers commented Sep 21, 2013

(sorry I pressed the wrong button)

@pahlers
Copy link
Author

pahlers commented Sep 23, 2013

Realname: Peter Ahlers

(I see you are an alpinist and climbed the Liskamm. I tried the Dufourspitze but had to return because the strong wind)

@pahlers
Copy link
Author

pahlers commented Sep 23, 2013

Changing the whole encoding system is a bridge to far for me.

I'm just using Angular for a month now. I need to learn a lot about writing unittests. But I don't mind to discuss the new functions.

@petebacondarwin
Copy link
Member

@eonlepapillon - the CLA reads like Chinese to me too and I am a native English speaker!

@petebacondarwin
Copy link
Member

OK, so my only objection to this is that it appears that while semi-colons are valid characters to appear in a URI segment (and query) they should really only be used for specific application purposes and otherwise still escaped.

In other words, if the semi colon appears in the content of the URI segment and is not being used as a delimiter then it should be escaped. The trouble right now is deciding at what point one should escape such things.

@pahlers
Copy link
Author

pahlers commented Sep 23, 2013

Good thinking.

A possible solution is to config these rules in the $resource configuration. I think there are two ways to configure.

uri part

The challenge here is that a $resource Object doesn't have a general configuration (only a configuration per method).

$resource('http://www.domain.com/:path?query=true#hash',{path:'Hello;World$'}, {
  get:{
    method: 'GET',
    ignoreEncode: {
     hierarchicalpart: ':',
     path: '$&:;' // string with characters to ignore during encoding
    }
  }
});

resource param

$resource('http://www.domain.com/:path?query=true#hash',{
  path: {
    ignoreEncode: '$&:;',
    value: 'Hello;World$'
  }
  }, {
  get:{
    method: 'GET'
  }
});

Maybe there are more solutions, but both way means a lot of refactoring.

@IgorMinar
Copy link
Contributor

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@pahlers
Copy link
Author

pahlers commented Dec 9, 2013

I'm sorry, It's not clear for me what you mean. I need to commit something and put my email in the commit?

@petebacondarwin
Copy link
Member

@eonlepapillon - your email is in the commits that you have provided in this PR. (See https://github.com/angular/angular.js/pull/4067.patch) We have used this to look for your signature of the CLA. Since we did not find it, @IgorMinar is suggesting that you have either not signed it or signed it with a different email address to the one in the commit.

@pahlers
Copy link
Author

pahlers commented Dec 9, 2013

@petebacondarwin - Thanks, I didn't know my email is visible in the patch.

@IgorMinar - I filled in a new CLA.

@pahlers
Copy link
Author

pahlers commented Dec 12, 2013

@IgorMinar Did you get my CLA?

@pahlers pahlers added cla: no and removed cla: yes labels Feb 19, 2014
@pahlers pahlers added cla: no and removed cla: yes labels Mar 19, 2014
@pahlers pahlers added cla: no and removed cla: yes labels Mar 31, 2014
@petebacondarwin
Copy link
Member

Closing due to #8377

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

Successfully merging this pull request may close these issues.

None yet

5 participants