Moved configuring to service provider.#1
Conversation
| if (config('google-analytics-4-measurement-protocol.measurement_id') === null | ||
| || config('google-analytics-4-measurement-protocol.api_secret') === null | ||
| ) { | ||
| throw new Exception('Please set .env variables for Google Analytics 4 Measurement Protocol as per the readme file first.'); | ||
| } |
There was a problem hiding this comment.
Does this mean that someone will face this exception for any request just after installing the package until they set the .env vars?
There was a problem hiding this comment.
This called only on resolving 'ga4'. Same as in the constructor.
| function () { | ||
| $clientId = session(config('google-analytics-4-measurement-protocol.client_id_session_key')); | ||
| if (empty($clientId)) { | ||
| throw new Exception('Please use the package provided blade directive or set client_id manually before posting an event.'); | ||
| } | ||
|
|
||
| return $clientId; | ||
| } |
There was a problem hiding this comment.
What is the benefit of passing the closure from here instead of keeping the login inside the facade?
There was a problem hiding this comment.
Service class should not know about configuration. But provider must configuring service.
|
Thanks for the PR Dmytro.
Maybe we can have a separate PR for this to keep things simple? |
|
Reverted "Also need to send additional user parameters with events like user_id." Simplified using only clientId for allow Closure for dynamic resolver for clientId |
dkulyk
changed the title
|
A bit occupied these days. Will revert after 17th May, thanks. |
|
@dkulyk I haven't added any tests in the package yet. Reason - I wasn't sure how to. Do you have any ideas? |
2 participants
GA4MeasurementProtocol required configuration for using. This is a problem for using it manually without package configuring.
Also need to send additional user parameters with events like user_id.