search

theiconic / php-ga-measurement-protocol

Send data to Google Analytics from the server using PHP. Implements GA measurement protocol.
MIT License
656 stars 135 forks source link

setAsyncRequest implementation is broken #44

Open borkor opened 7 years ago

borkor commented 7 years ago

After upgrading to v2.* setAsyncRequest has completely blocked complete execution of $analytics->setAsyncRequest(true)->sendPageview();

I believe that this is might be the issue related to usage of guzzlehttp. Had to completely disable asynchronous execution. Any thoughts on this?

jorgeborges commented 7 years ago

It may have to do with the guzzlehttp implementation, yes. This was done in this PR: https://github.com/theiconic/php-ga-measurement-protocol/pull/6

A quick google of the issue guided me to this thread where I see you commented as well, I can see in our code that we are not using that tick method mentioned. https://github.com/guzzle/guzzle/issues/1127

I'll look at this over the next few days, but if you think you can fix it feel free to open a PR for it.

borkor commented 7 years ago

Asynchronous method should removed from future releases.

There are a whole bunch of issues in guzzle implementation regarding async requests. As it is now in php-ga-measurement-protocol it is misleading and making people believe this works as reactphp/promise.

Down to the point. Curl implementation in PHP is not asynchronous.

jorgeborges commented 7 years ago

I'll remove it from the current README for the time being...

I'm pretty sure this was working as I test it personally in v1 using Guzzle 5, I think it was indeed using ReactPHP along with promises, check out the composer.lock to see that it was being required before https://github.com/theiconic/php-ga-measurement-protocol/blob/v1/composer.lock

However, it seems to have broken when we upgraded to Guzzle 6, if you check the current composer.lock, React is not there anymore.

I found a good article on how to make Guzzle 6 async using React: http://stephencoakley.com/2015/06/11/integrating-guzzle-6-asynchronous-requests-with-reactphp

But then again, I also want to implement Http Plug to not couple with Guzzle anymore http://httplug.io/

I'll study these options and see if I fix it or remove it altogether from the next big release. Thanks @borkor for reporting the bug.

yellow1912 commented 6 years ago

In general, you have to call wait() for the async requests with Guzzle (which kinda destroy the purpose)....

lapswr commented 5 years ago

so this bug still exists, I guess it's possible to fix?

Daniel15 commented 5 years ago

If async isn't working, the approach I'd take is to send all your analytics events after flushing the response, so that they don't slow down the page response time. If you're using PHP-FPM, you can flush the response using fastcgi_finish_request(). This sends the response to the client and closes the connection, but still lets you execute some other PHP code.