Skip to content

Conversation

@olampert
Copy link
Contributor

Pull Request Checklist

Please complete the following before submitting:

General notes -

  • I have tested the changes locally.
  • I have written unit tests where applicable.
  • I have updated documentation where needed.
  • I have added comments to complex code.
  • This PR follows the coding style guidelines.
  • I have updated release notes with new feature

New Kaltura Types

  • I have created new clients
  • I have notified related apps - KMCNG / KMS / EP .... about new clients

New Kaltura Services / Actions

  • I have added a deployment script

Questions

  1. What is the purpose of this PR?

    • Enter your answer here
  2. Does this change affect production code or infrastructure?

    • Yes
    • No
  3. If yes, what is the rollback plan?

    • Enter your answer here

@github-actions
Copy link

@github-copilot suggest

use requestUtils::sendAnalyticsBeacon
@github-actions
Copy link

@github-copilot suggest

}

protected static function getCustomReportAnalyticsEventContent($id, $params, $partner_id)
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding sanitization and size limits to $params

{
$stats_host = explode(':', kConf::get(self::INTERNAL_ANALYTICS_HOST));
$customReportEventContent = self::getCustomReportAnalyticsEventContent($id, $params, $partner_id);
requestUtils::sendAnalyticsBeacon(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can sendAnalyticsBeacon throw exception? if so please wrap it with try/catch and proper handler

$api_info = '';
foreach ($params as $key => $value)
{
if ($key != 'partner_id')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const for partner_id


if (kConf::hasParam(self::INTERNAL_ANALYTICS_HOST))
{
$stats_host = explode(':', kConf::get(self::INTERNAL_ANALYTICS_HOST));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets validate that $stats_host array length is 2 to avoid exception

requestUtils::sendAnalyticsBeacon(
$customReportEventContent,
$stats_host[0],
isset($stats_host[1]) ? $stats_host[1] : 80
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const for default http port 80

@github-actions
Copy link

@github-copilot suggest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants