mage.cookies.set/get fix for value object

[iazel]

When we pass an object as value to mage.cookies.set, it incorrectly converts to string and the result is something like: "[Object object]".
This means that when we enable Cookie Restriction Mode, the method CookieHelper::isUserNotAllowSaveCookie will always return false, breaking other modules such as Google Analytics (the monitoring code will never show up!).

This PR fix the problem

[orlangur]

When we pass an object as value to mage.cookies.set

Does this happen somewhere in core code? You can see here that value is not supposed to be an object.

[iazel]

@orlangur Yeah, as I said, it happens when Cookie Restriction Mode is on.

1. Prepare value
2. Embed it in the page
3. Set on click

[orlangur]

@lazel 1. returns JSON encoded string, so, I don't see how it can cause a trouble.

My whole point is that any code which does not pass string as cookie value must be fixed instead of complicating library.

[iazel]

@orlangur Yeah, point 1 do a json_encode, but that's because point 2 embed it in Javascript, so the final result on the client side is an object.

I don't think this overly complicate the library, but instead make it less error-prone and more developer-friendly. I mean, if the core dev team stumbled on this, let alone others ^^"

[orlangur]

@lazel didn't get about 2 yet, to me it looks like result will be

"cookieValue": "someString"

which string is then passed.

less error-prone and more developer-friendly

Well, it just hides the problem in non-library code which does not follow contract ("pass only string value"). As to me, fail-fast is the most developer-friendly strategy, so, instead of changing library logic which will also break all previously set non-encoded cookie values I would add an assertion like

if (value is not string) {
    throw exception;
}

and fix invalid non-library code.

[iazel]

@orlangur Sorry for being blunt, but check all the code before asserting something. The final result, from the JS point of view is like:

"cookieValue": {"1": true}

That's said, throwing an exception could be a good solution too.

[orlangur]

@lazel still don't understand https://3v4l.org/QrkD9, I'll a try this piece of Magento code in action better.

[iazel]

@orlangur Please read more carefully the line on point 2. When you embed the string inside the template, the final result that the browser will receive and parse is the one I wrote earlier. In your snippet you have to use echo instead of var_dump.

However, in a second thought, I think your proposition of throwing an exception and fixing the bad case will have a better impact on older code, so I'm going to rebase this PR :)

[orlangur]

Thanks @lazel (handshake)

[vrann]

@iazel please update the branch with the latest develop branch.

[orlangur]

Just to clearify: initial implementation DOES NOT LGTM

However, in a second thought, I think your proposition of throwing an exception and fixing the bad case will have a better impact on older code, so I'm going to rebase this PR :)

this one seems a good one 👍

[orlangur]

Closing this PR for now. Feel free to reopen if you would like to continue at any time.
Thank you for collaboration.