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

Check tweet json for rouge newlines to handle multiple json objects #2

Closed
wants to merge 2 commits into from

Conversation

lwward
Copy link

@lwward lwward commented May 21, 2021

Adding a second check for newlines helps fix issue with multiple json objects trying to be parsed by json_decode

Fixes #1

@felixdorn
Copy link
Owner

felixdorn commented May 25, 2021

Thanks for your contribution!

I'm not sure how did you manage to get multiple json objects because as soon as the stream contains one \r\n, it yields the result so please provide more detailed context.

src/TwitterStream.php Outdated Show resolved Hide resolved
@lwward
Copy link
Author

lwward commented May 25, 2021

Thanks for your contribution!

I'm not sure how did you manage to get multiple json objects because as soon as the stream contains one \r\n, it yields the result so please provide more detailed context.

I believe it's because you are reading two characters at a time so the \r\n don't alway get grouped together. Below are a couple of examples to try and explain my thinking...

This example works:
{" na me ": "f el ix "} \r\n {" na me ": "s am "}

This one doesn't:
{" na me ": "l uk e" }\r \n{ "n am e" :" jo hn "}

@felixdorn
Copy link
Owner

felixdorn commented May 25, 2021

Yeah! That makes sense, good catch!

There's just one thing: your solution will (potentially) store n tweets with an non even length until there's one with an even length which could result in an enormous string and tweets not being delivered in real time which is a big no-no.

It'd be better if we just take the current character and the last character of the $tweets variable and check if the two combined are \r\n.

Thanks for contributing!

@lwward
Copy link
Author

lwward commented May 25, 2021

Ah yes, good point. That's a much better solution

@felixdorn
Copy link
Owner

Fixed by d8e80d6 with the implementation we discussed

@felixdorn felixdorn closed this May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More than one tweet object can be loaded from the stream at once
2 participants