Don't check cookie value if cookie was deleted #17830
Conversation
sgiehl
added
not-in-changelog
|
is it possible to test this? |
|
Does the test fail if any errors are logged during the execution? |
|
@Findus23 I'm not sure, but unless an exception is thrown, I doubt it. |
| @@ -2449,7 +2449,7 @@ if (typeof window.Matomo !== 'object') { | |||
| ';SameSite=' + sameSite; | |||
|
|
|||
| // check the cookie was actually set | |||
| if (getCookie(cookieName) !== value) { | |||
| if ((!msToExpire || msToExpire >= 0) && getCookie(cookieName) !== value) { | |||
There was a problem hiding this comment.
should it be msToExpire >= 1 or >= 5 maybe as the value might be expired by the time it's set and read? I suppose we don't set such values so it doesn't really matter.
There was a problem hiding this comment.
Guess no one should use such low values for expire time. If someone does nevertheless, might be good to have a warning, as it simply might not make any sense to set a cookie that expires before it can be used 🙈
|
@diosmosis @Findus23 Within the javascript tests |
|
Tried writing some tests for a while now, but for some reason the error message is not triggered when I would expect it (like setting an invalid cookie domain). It's only triggered when a cookie is deleted, which should be fixed with the changes here... |
|
@sgiehl if the test fails in 4.x-dev then it's still useful, right? If it's not possible though then I guess it's not vital. |
|
maybe we should consider adding some proper tests for the cookie methods
later. For some reasons they didn't trigger a console error when I would
have expected it. But maybe my expectations were wrong. Feel free to have a
look yourself Dizzy if you have some time today...
|
…ed w/ valid input
|
Added a test that fails on 4.x-dev but passes on this branch. Will merge now. |
Description:
The issue is reproducible calling
_paq.push(['setCookieDomain', 'custom.matomo.dev']);, which tries to set and remove a cookie on the given domain.fixes #17829
Review