Move methods away from Live controller #12406
Conversation
sgiehl
added
Needs Review
plugins/Live/Live.php
Outdated
| * @return ProfileSummaryAbstract[] | ||
| * @throws \Exception | ||
| */ | ||
| public static function getAllProfileSummaryInstances() |
There was a problem hiding this comment.
Ideally this would be moved into a class named eg LiveProvider or ProfileProvider + not be static to be consistent with core. Also applies to getAllProfileSummaryClasses .
The plugin class itself should have only a few lines of code related to events/hooks...
plugins/Live/Live.php
Outdated
| @@ -114,4 +122,91 @@ public function renderVisitorIcons(&$renderedDetails, $visitorDetails) | |||
| $renderedDetails .= $instance->renderIcons($visitorDetails); | |||
| } | |||
| } | |||
|
|
|||
| public static function getSegmentWithVisitorId() | |||
There was a problem hiding this comment.
Maybe this could be moved into a different class outside the Plugin class?
Also is the cache needed here or can we maybe use transient cache so it will be also invalidated between tests etc?
There was a problem hiding this comment.
Not sure where it would be useful to move that method to.
I guess the cache is there to prevent Request::processRequest('Live.getMostRecentVisitorId') being called multiple times with different results. Guess using transient cache should be fine as well...
There was a problem hiding this comment.
Be very useful to add this as a comment 👍 as we will otherwise eventually introduce a bug here when someone removes the caches (it looks like it isn't needed.)
|
@tsteur would you have some time for another review on this? Would be good to merge some more of the open PRs |
|
Just tested and worked. Be good to add the other comment, otherwise good to merge. |
|
Ok. I've added the comment. Will now wait for the tests before merging... |
* move methods away from Live controller * Add new class ProfileSummaryProvider * use transient cache instead of static variable
fixes #12394