Skip to content
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

Cannot extends Analytics Class. #53

Open
yondifon opened this issue May 16, 2023 · 13 comments · May be fixed by #54
Open

Cannot extends Analytics Class. #53

yondifon opened this issue May 16, 2023 · 13 comments · May be fixed by #54
Assignees
Labels
question Further information is requested

Comments

@yondifon
Copy link

Hello .

Thank you for the work on this.
I have an issue extends and I think I have a lead.

These should protected right ??

private BetaAnalyticsDataClient $client;
private RequestData $requestData;

For this to work

laravel-analytics/README.md

Lines 545 to 567 in 1cdbc19

```php
namespace App\Analytics;
use Gtmassey\LaravelAnalytics\Analytics;
use Gtmassey\LaravelAnalytics\Request\Filters\Filter;
use Gtmassey\LaravelAnalytics\Request\Filters\FilterExpression;
use Gtmassey\LaravelAnalytics\Request\Metrics;
class CustomAnalytics extends Analytics
{
public function onlySessionsAbove(int $count): static
{
$this->metricFilter(fn(FilterExpression $filterExpression) => $filterExpression
->filterMetric(
metricsCallback: fn(Metrics $metrics) => $metrics->sessions(),
filter: fn(Filter $filter) => $filter->greaterThanInt($count),
)
);
return $this;
}
}
```

@Plytas
Copy link
Collaborator

Plytas commented May 16, 2023

Hello @yondifon.

I don't believe those properties should be protected as the example provided in README works.

Can you explain your use case and what issues you encounter? Are you trying to access $client and/or $requestData properties on your class that extends Analytics?

@Plytas Plytas self-assigned this May 16, 2023
@Plytas Plytas added the question Further information is requested label May 16, 2023
@yondifon
Copy link
Author

I am trying to extends the analytics class and add https://github.com/GoogleCloudPlatform/php-docs-samples/blob/main/analyticsdata/src/run_report_with_ordering.php#L6

With Private properties the child can't use that parent property.

Hello @yondifon.

I don't believe those properties should be protected as the example provided in README works.

Can you explain your use case and what issues you encounter? Are you trying to access $client and/or $requestData properties on your class that extends Analytics?

@Plytas
Copy link
Collaborator

Plytas commented May 17, 2023

Gotcha. Our plan is to not expose underlying properties, but to provide an expressive and fluent way to interact with them. Otherwise, people could just use Google package.

I'm planning to have a PR ready later today or tomorrow. Thanks for bringing this to our attention.

@Plytas
Copy link
Collaborator

Plytas commented May 17, 2023

@yondifon while I'm working on this, could you provide an example order by you'd like to perform? I will then provide a snipped of how it would look with our package to get your feedback.

@yondifon
Copy link
Author

public static function getTopPages(?Period $period = null): ResponseData

An example right here, we want to get the top 10 visited pages.

  1. So Order by Metrics ( think it's really important)
  2. Dimensions will be nice too

🤔 And now that I have gone through this, I don't think this is correct.

  • If you have a limit of 10 pages and 11 pages, what if the 11th page has more views
  • The title of a page can change, but that doesn't mean the page changes

@Plytas
Copy link
Collaborator

Plytas commented May 17, 2023

It should order by metric by default, at least that's what I'm getting on my end. Have you tried this out?

In case of getTopPages() this pre-made report only considers page titles, and yes 2 different titles for the same page would return separate rows. That's how Google Analytics treats it. If you want you could use pagePath() dimension instead.

In terms of explicitly specifying ordering (metrics and/or dimensions), would this satisfy your needs?

$report = Analytics::query()
    ->forPeriod(Period::defaultPeriod())
    ->setDimensions(fn(Dimensions $dimensions) => $dimensions->pageTitle())
    ->setMetrics(fn(Metrics $metrics) => $metrics->sessions())
    ->setOrderBys(fn(OrderBy $orderBy) => $orderBy
    
        // Order by metric
        ->metricDesc(
            metricsCallback: fn(Metrics $metrics) => $metrics->sessions(),
        )
        
        // or order by dimension
        ->alphanumericDimensionAsc(
            dimensionsCallback: fn(Dimensions $dimensions) => $dimensions->pageTitle(),
        )
    )
    ->run();

You could chain any number of order by rules (as many as Google Analytics API allows). For metrics you'd have these 2 options:
image
For dimensions you'd have 6 options:
image

@yondifon
Copy link
Author

Yes. It does.

Thank you for this.

@Plytas
Copy link
Collaborator

Plytas commented May 17, 2023

Alright, I'll ping again once PR is ready for you to try it out.

@Plytas Plytas linked a pull request May 17, 2023 that will close this issue
@Plytas Plytas linked a pull request May 17, 2023 that will close this issue
@Plytas
Copy link
Collaborator

Plytas commented May 17, 2023

@yondifon I created a PR #54. Would be nice if you could test it out before the release by running composer require gtmassey/laravel-analytics:dev-implement-order-by.

@yondifon
Copy link
Author

Alright. Thanks.

I'll look it later today.

@gtmassey
Copy link
Owner

Thanks for this! I've been busy with work so I haven't had a lot of time to review or pay attention to this recently.

@Plytas
Copy link
Collaborator

Plytas commented May 25, 2023

@yondifon Have you had time to test this out yet?

@yondifon
Copy link
Author

Hey @Plytas

Got swamped up with something else. I'll have a proper look this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants