Again: "Urlencode sku of product listing.""

[skwp]

This is a re-PR of @zztimur 's code

[skwp]

Here's the actual example. In magento I entered a sku:

test1234 with spaces!?

With your new code the sku in reverb ended up as

test1234+++with+spaces%21%3F

With the old code, the sku on reverb ends up correct.

I think you are correctly pointing out some kind of bug here, but I don't think urlencoding the sku at least in the place you've done it is the correct solution.

[skwp]

This curl request does not look correct: curl -i -k -XGET "https://sandbox.reverb.com/api/my/listings?state=all&sku=test1234+++with+spaces%21%3F"

[skwp]

I'm pretty sure your fix should be to urlencode the GET request but NOT to urlencode the sku in the post to reverb. Post body params do not need to be urlencoded; they are not urls.

[skwp]

In other words, the correct implementation would be,

1. when POST/PUT the sku to reverb, use the unencoded sku in the post/put body "sku foo bar"
2. when GET the listing by sku, urlencode the SKU in the GET lookup because urls should be urlencoded. It is more correct to urlencode the entire url than just the sku as you may have spaces in other places.

[dunagan5887]

Would it be possible to URL decode the GET request on the Reverb side so that Reverb is storing the expected sku?

[zztimur]

@dunagan5887 Yes, that's what I initially thought. I don't know how fast reverb can implement it.

[skwp]

I don't think this is an API level responsibility. Some people may legitimately want a "+" in their SKU. There would be no way for us to know whether the sku is url encoded. URL encoding something that clients should always do for GET requests. That really should be done at the curl adapter layer and I'm surprised it's not handled for you.

[dunagan5887]

I'll double-check but the cURL adapter is URL encoding the sku, which is what's causing the problem.

During the POST/PUT request the sku is NOT url encoded, so Reverb does not recognize it as existing in the Reverb system.

[skwp]

I think we still have a miscommunication here. The sku in the body of PUT/POST should not ever be urlencoded. urlencoding is for urls only, each GET request should be url encoded on the url level, not a per-attribute level

[skwp]

Without the zztimur fix to me it looks like everything is correct. The outbound requests based on the log show the sku being urlencoded, which is right. I'm not sure what zztim's fix is fixing

[dunagan5887]

"Some people may legitimately want a "+" in their SKU"

Then the following would be sent for sku "test+1234 with + spaces!?"

"https://sandbox.reverb.com/api/my/listings?state=all&sku=test%2B1234+with+%2B+spaces%21%3F"

"each GET request should be url encoded on the url level, not a per-attribute level"

encoding the entire url would result in the following, which definitely would not resolve correctly

https%3A%2F%2Fsandbox.reverb.com%2Fapi%2Fmy%2Flistings%3Fstate%3Dall%26sku%3Dtest%2B1234+with+%2B+spaces%21%3F

[skwp]

From chatting with timur I think there might be some edge case where certain chars like tabs or other nonprintables might not be handled correctly by the put/post. However I think that's more of a problem with the client's magento data than something we can really handle. Let me know if there's some workaround for that but I'm not sure what to do with a sku that has a tab in it.

[skwp]

you might be right about encoding the entire url. I was basing that off of ruby's URI.encode which correctly encodes just the parts that need to be encoded. php's urlencode might not be that smart :)

[dunagan5887]

If you run a url decode on whatever is passed in the sku field, this should solve everything. I'm not sure what issues would be caused by this

PHP is better than Ruby :P

[skwp]

we can't urldecode because that would turn "sku+foo" into "sku foo". some people use plusses for a reason, or other special chars.

[dunagan5887]

So let's say a product's sku is "sku+foo". Then the GET request would communicate "sku%2Bfoo" Running a URL-decode on "sku%2Bfoo" results in "sku+foo" which is the correct sku

[skwp]

correct. we already do the urldecode in transit, so everything is working as expected, without zztimur's changes. I am closing this PR for now until we can pinpoint the problem. I think it has to do with nonprintable characters.

[zztimur]

why does url request with "sku%9" doesn't match the product with "sku\t" set in reverb?

On Wed, Apr 27, 2016 at 3:50 PM, Yan Pritzker notifications@github.com wrote:

Without the zztimur fix to me it looks like everything is correct. The outbound requests based on the log show the sku being urlencoded, which is right. I'm not sure what zztim's fix is fixing

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/reverbdotcom/reverb-magento/pull/242#issuecomment-215224898

-- Sent from my iPhone

[zztimur]

Safe to assume you guys will be handling this on server side?

[kylecrum]

@zztimur why does the sku have an unprintable character in it? That's just a recipe for all kinds of errors. From a UX point of view, someone is searching for their sku in a UI, they would then need to know that there is a tab at the end?

[skwp]

@zztimur I just tried to urlencode "sku\t" here http://meyerweb.com/eric/tools/dencoder/ and got sku%5Ct so perhaps the problem is the %9

[skwp]

i see that CGI escape produces "sku%09". but "sku%9" is not valid. there should be a 2 digit code there

[zztimur]

Kyle, I understand. But magento doesn't validate that, so users put all sorts of things.

On Wed, Apr 27, 2016 at 4:14 PM, Yan Pritzker notifications@github.com wrote:

@zztimur https://github.com/zztimur I just tried to urlencode "sku\t" here http://meyerweb.com/eric/tools/dencoder/ and got sku%5Ct so perhaps the problem is the %9

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/reverbdotcom/reverb-magento/pull/242#issuecomment-215231361

-- Sent from my iPhone