Add new segment ActionUrl + new operators starts with and ends with #9228
Conversation
tsteur
added
not-in-changelog
| { | ||
| // segment metadata | ||
| if (empty($this->availableSegments)) { | ||
| $this->availableSegments = API::getInstance()->getSegmentsMetadata($this->idSites, $_hideImplementationData = false); |
There was a problem hiding this comment.
This shouldn't change between different Segment instances correct? Perhaps this logic can be moved to an object in DI so API.getSegmentsMetadata is not called more than once? Actually, maybe API.getSegmentsMetadata could store the result in a cache?
There was a problem hiding this comment.
It can change between instances re the idSites. Don't think there is need for a cache for now.
There was a problem hiding this comment.
Actually I might need similar feature somewhere else and might introduce a class for this. Will do it in another branch then as this one might be merged sooner. Problem is that core/Segment is a bit broken by design as it is in core but accesses the API. It should actually not access anything from a plugin... this would require lots of refactoring though I reckon
There was a problem hiding this comment.
You could include the idsites in the cache key. I think it's possible for multiple segments to be created with the same idsites but different definition in one request.
There was a problem hiding this comment.
I don't think it's worth it. When this class is used, it means we're fetching a report or something and then through the whole request we only work on one idSite. Or to describe it differently during one request it should not be called with different idSites. Also do not really see the advantage of putting it in DI apart from a possible caching. I'd rather add caching when there is an actual performance problem or so
|
Left some comments, needs a rebase, otherwise looks good to me. |
|
I just noticed the new operators are actually not available in the segment selector yet for dimensions, I will change this in a bit |
|
FYI: I just moved the check to the Segment data class and made the new operators available in the UI. Next: rebase |
Add new segment ActionUrl + new operators starts with and ends with
fixes #9224
fixes #8076
I know it would have been better to issue different PRs for these two issues but to validate whether we actually need #8076 and #9225 I had to develop them kinda at the same time.
There is a new feature to combine multiple segments via
ORcalledunionOfSegments. I also used them for custom variables already. I would have used the existingsetSqlSegments(array(...))but it was not possible to use this for action urls as they are referenced withlog_actionunder differenttypeIDs.The UI will now show a title in the segment editor segment list on hover if a segment is a union of different segments (eg hover
actionUrlorCustom Variables Name)