@sgiehl opened this Pull Request on July 28th 2021 Member

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

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on July 28th 2021 Member

is it possible to test this?

@Findus23 commented on July 28th 2021 Member

Does the test fail if any errors are logged during the execution?

@diosmosis commented on July 28th 2021 Member

@Findus23 I'm not sure, but unless an exception is thrown, I doubt it.

@sgiehl commented on July 29th 2021 Member

@diosmosis @Findus23 Within the javascript tests setCookieDomain is used, and it actually throws that error on console multiple times. But those errors are ignored and might be hard to test. Guess we would need to overwrite console.error and console.log in order to be able to get and compare the log message. Will check if that's fast to implement...

@sgiehl commented on July 29th 2021 Member

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...

@diosmosis commented on July 29th 2021 Member

@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.

@sgiehl commented on July 29th 2021 Member

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...

@diosmosis commented on July 29th 2021 Member

Added a test that fails on 4.x-dev but passes on this branch. Will merge now.

This Pull Request was closed on July 29th 2021
Powered by GitHub Issue Mirror