New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move methods away from Live controller #12406
Conversation
plugins/Live/Live.php
Outdated
* @return ProfileSummaryAbstract[] | ||
* @throws \Exception | ||
*/ | ||
public static function getAllProfileSummaryInstances() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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