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

WP_Stream_DB::store() and WP_Stream_Log::log() don't return true/false correctly #711

Closed
sc0ttkclark opened this issue Mar 21, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@sc0ttkclark
Copy link

commented Mar 21, 2015

I'm finding it hard to determine if a Stream event has been logged from within a Stream connector, or any other place.

Can you pass true/false based on the $result in WP_Stream_DB::store() and ensure that true/false gets passed down to WP_Stream_Log::log() which it's called from?

return true; after the do_action in WP_Stream_DB::store(), and return false; below that statement because it would have not been successful ($result is empty or is_wp_error).

In WP_Stream_Log::log you could set $stored = WP_Stream::$db->store( array( $recordarr ) ); and return $stored; at the end of the method.

@sc0ttkclark

This comment has been minimized.

Copy link
Author

commented Mar 21, 2015

You'll also want to update the log method @return phpdoc, since it says void currently. The store() method says it returns boolean based on if it saved properly, so that is already set as expected.

@fjarrett fjarrett closed this in 509196b Apr 2, 2015

@fjarrett

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2015

@sc0ttkclark Thanks for pointing this out. Does this work for you?

Also, just as a side note: When the actual record is sent off to the WP Stream API for storage it's sent using a non-blocking request, so we can't actually verify if it saved or not. This would only be a problem for malformed records that the API would reject due to invalid data.

@sc0ttkclark

This comment has been minimized.

Copy link
Author

commented Apr 2, 2015

Gotcha, yeah this is concerning when planning on using Stream for logging more activity on a site for an item, with the possibility of data loss when the API becomes unavailable for whatever reason.

@sc0ttkclark

This comment has been minimized.

Copy link
Author

commented Apr 2, 2015

Maybe WP_Stream_DB::insert could get $blocking added and passed into WP_Stream_API::new_records, which WP_Stream_DB::store could set $blocking through a filter, which gets passed $records. The filter would get placed after the $records themselves are filtered and confirmed not empty.

@sc0ttkclark

This comment has been minimized.

Copy link
Author

commented Apr 9, 2015

I can't reopen or I would, any way to address the $blocking enhancement so this could be used in a more precise way (when doing logging from crons which have no timeout / UI limitations)

@fjarrett

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2015

@sc0ttkclark I think this thread and the blocking enhancement, while related, are really two separate issues.

@sc0ttkclark

This comment has been minimized.

Copy link
Author

commented Apr 9, 2015

I'll open a new issue then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.