Skip to content
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

tests and fixes for set_doku_pref issues #2722

Merged
merged 6 commits into from Mar 30, 2019
Merged

tests and fixes for set_doku_pref issues #2722

merged 6 commits into from Mar 30, 2019

Conversation

phy25
Copy link
Collaborator

@phy25 phy25 commented Mar 12, 2019

A fix for #2721.

  • PHP tests added
  • Read last entries (if duplicate) when get
  • Remove duplicate entries when set
  • Type-aware compare to decide append or modify
  • Remove $cookieVal empty check before setcookie, because there might be cases when we want to remove cookie

Also a JavaScript cookie patch: Convert value type to string to prevent different type being returned by DokuCookie.getValue().

DokuCookie.setValue('foo', false);
console.log(DokuCookie.getValue('foo')); // buggy false, should be 'false'
location.reload(); // pseudo code
console.log(DokuCookie.getValue('foo')); // 'false'

AppVeyor is failing just because it doesn't like styleutils_cssstyleini_test::test_mergedstyleini in another PR.

- Read last entries (if duplicate) when get
- Remove duplicate entries when set
- Type-aware compare to decide append or modify
- Remove $cookieVal empty check before setcookie, because there might be cases when we want to remove cookie
This prevents different type returned for DokuCookie.getValue():

DokuCookie.setValue('foo', false);
console.log(DokuCookie.getValue('foo')); // false
location.reload(); // pseudo code
console.log(DokuCookie.getValue('foo')); // 'false'
@splitbrain
Copy link
Collaborator

The PHP part looks good, I'm not so sure about the JavaScript. Ideally the PHP and JavaScript should behave exactly the same. So setting something to false should delete the cookie instead of setting it to 'false' shouldn't it? And we probably would need to add the second parameter for setting a default to DokuCookie.getValue(). Are we relying on false being turned to 'false' somewhere?

@phy25
Copy link
Collaborator Author

phy25 commented Mar 13, 2019

So setting something to false should delete the cookie instead of setting it to 'false' shouldn't it?

Didn't realize this when I use false instead of true as an example. However, I am not sure if this is a breaking change.

And we probably would need to add the second parameter for setting a default to DokuCookie.getValue().

Currently it returns undefined. I am for it.

Are we relying on false being turned to 'false' somewhere?

I am not sure why false->'false' is relied on currently. If we read from cookies, DokuCookie.getValue() always returns string, so this patch makes it consistent when DokuCookie.setValue() and DokuCookie.getValue() are being run in the same page session.

@Klap-in
Copy link
Collaborator

Klap-in commented Mar 13, 2019

Current usage according to Codesearch:

@phy25
Copy link
Collaborator Author

phy25 commented Mar 13, 2019

I went through DokuCookie.setValue and DokuCookie.getValue in codebase, and it looks like developers use "" a lot. Nobody uses false.

So...

  • Setting something to false should delete the cookie instead of setting it to 'false' - fe36a27
  • add the second parameter for setting a default to DokuCookie.getValue() - bded2f5

As I mentioned, the only thing that hasn't been addressed yet, is that I don't understand "Are we relying on false being turned to 'false' somewhere"...

@splitbrain
Copy link
Collaborator

What I meant with "Are we relying on false being turned to 'false' somewhere" is that I wasn't sure if any plugin maybe sets a cookie to false and expects to get a string of 'false' later on to do something with that. Judging from the code search results nobody seems to do that, so this should be good.

@splitbrain splitbrain merged commit 0215a24 into master Mar 30, 2019
@splitbrain splitbrain deleted the issue2721 branch March 30, 2019 20:42
@Klap-in
Copy link
Collaborator

Klap-in commented Jun 8, 2020

Manual tests for javascript: see #2721 (comment)

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

Successfully merging this pull request may close these issues.

None yet

3 participants