Open RFreij opened 3 years ago
Thanks!
As I already said, I don't like this "magic" implementation. I understand your needs but I would prefer control over automation.
We don't know any users needs. Maybe this automation could create some unwanted situation.
Thanks for the PR @RFreij.
To be honest, I don't have hard feelings on either approach. I would prefer the auto event flush just because that seems to make the library easier to use, but on the other hand, @alberto-bottarini did point out the rest of the library throws exceptions when it's used wrong (such as when not all required data for a hit is included in the Analytics object), so having it throw the Exception makes things consistent.
Another approach would be to make this configurable, by default it doesn't auto send the batch, but if you call a method called enableAutoBatchHitSend
or the like, then that activates the auto flush.
I think we can leave this one up to the community to decide, I'll create an issue, @ all the people who has contributed to the library, and depending on the number of votes using 👍 or 👎 we'll decide.
... or add a wrapper method called enqueOrSend<hit-type>()
which also gets rid of any "magic".
PS: I want the hasEnqueuedUrls()
method in this PR :)
PPS: last but not least, if this PR gets rejected there is still the possibility of extending the Analytics
class:
<?php
use TheIconic\Tracking\GoogleAnalytics\Analytics;
class AnalyticsDriver extends Analytics
{
protected $autoFlushEnabled = false;
public function enableAutoFlush()
{
$this->autoFlushEnabled = true;
return $this;
}
public function disableAutoFlush()
{
$this->autoFlushEnabled = false;
return $this;
}
protected function enqueueHit($methodName)
{
if (count($this->enqueuedUrls) == 20 && $this->autoFlushEnabled) {
$this->sendEnqueuedHits();
}
return parent::enqueueHit($methodName);
}
}
Hi, first of all thank you @alberto-bottarini for creating the batch functionality.
In the discussion of #83 @jorgeborges said he would prefer the batch to be sent automatically after the 20 enqueued url's have been reached. Personally I like this approach so this is why i'm creating this pull request.
I've also added a hasEnqueuedUrls method. This method allows us to determine if any enqueued url's are still available before sending the response back to the client. If any are available then send the enqueued hits. We're using Laravel so I created a middleware to do exactly that for each response.
This way we don't have to worry about the limit.
It would be nice to have at least the hasEnqueuedUrls method added. I understand the philosophy behind the exception approach as well, if you decide to keep it that way than i will create something myself.
I coudn't get phpunit to work, i got the following error:
I'll check them again if they do not pass.