Refactor search category/count to not use custom variables #15286
Conversation
katebutler
added
the
Pull Request WIP
|
Guess Kate won't finish this PR, so I will take over... |
sgiehl
force-pushed
the
12072
branch
4 times, most recently
from
c23465c
to
a442aa8
Compare
|
I've updated the PR and fixed some obvious bugs, adjusted the tests and also made search category and count available as segments Should be ready for a review now |
sgiehl
added
Needs Review
|
|
||
| if (in_array($cvarKey, ['_pk_scat', '_pk_scount'])) { | ||
| continue; // ignore old site search variables | ||
| } |
There was a problem hiding this comment.
Is it currently supported to send these custom variables manually in tracking requests? Would this break that use case?
There was a problem hiding this comment.
Actually it seems it wasn't restricted to send those variables manually. Not sure if that is something we should handle 🤔 Don't think someone used those variables accidentally.
There was a problem hiding this comment.
I guess our options are:
- just stop handling them
- report a tracking error if found so users can fix their tracking code
- keep handling them in some way (ie, not recording as custom var, but as search dimension value)
What do you think?
cc @tsteur
There was a problem hiding this comment.
I reckon we can keep the logic there as is and doubt indeed someone uses these values. And in worst case these values would just not be visible in the visitor log.
...PHPUnit/System/expected/test_ManyVisitorsOneWebsiteTest__Live.getLastVisitsDetails_month.xml
Show resolved
Hide resolved
|
There are some tests that are failing, including one UI test that looks non-trivial to fix, otherwise looks good to merge to me. |
|
@diosmosis the failing plugin tests simply needs to be update in the submodule. Would do that before merge. But which UI test are you referring to? I think all of those failing here might also fail on 4.x-dev. (some of them started failing after merging 3.x-dev to 4.x-dev) |
|
@sgiehl there was an actions report that was failing but I don't see it anymore, I'm not sure what happened there. I guess ignore my comment. |
Fixes #12072