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.

Sign up for GitHub

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

Jump to bottom

Forward compatebility with ReactPHP packages #51

Closed
wants to merge 6 commits into from
Closed

Forward compatebility with ReactPHP packages #51

wants to merge 6 commits into from

Conversation

WyriHaximus
Copy link

🎉

@WyriHaximus
Copy link
Author

I've contacted Travis about the missing build. Will comment here once that has been resolved.

@WyriHaximus
Copy link
Author

Issues resolved for Travis 🎉

@WyriHaximus
Copy link
Author

WyriHaximus commented Oct 26, 2017 •
edited
Loading

Failing job is due to travis error with PHP 5.3

@clue clue added this to the v2.0.0 milestone Oct 27, 2017
@clue
Copy link
Owner

clue commented Oct 27, 2017

Thank you for filing this PR, much appreciated!

I think your suggested changes make perfect sense and now that the Stdin and Stdout classes are pretty much just dummy wrapper objects, I wonder if it doesn't make more sense to remove them altogether and simply use the base streams instead. What do you think about this?

(Also thanks for looking into the tests, I'll make sure to take care of the legacy PHP tests)

@clue
Copy link
Owner

clue commented Oct 27, 2017

Unrelated legacy PHP build error has been addressed via #52 :shipit:

@WyriHaximus
Copy link
Author

Updated the PR to use streams directly instead of wrapping them in Stdin and Stdout.

@WyriHaximus
Ensure close is called
923b5f1
@WyriHaximus
Merge branch 'master' of https://github.com/clue/php-stdio-react into…
ead9089 
… async-STDOUT

# Conflicts:
#	src/Stdin.php
#	src/Stdio.php
#	src/Stdout.php
@mpociot
Copy link

mpociot commented Dec 30, 2017

@clue any chance of getting this merged?

@clue
Copy link
Owner

clue commented Dec 30, 2017

@mpociot Thanks for the friendly ping :) It's near the top of my current TODOs, expect to see a result some time next week 👍

@mpociot
Copy link

mpociot commented Dec 30, 2017

That’s great, thank you!
I’m using this as one of the dependencies of my BotMan chatbot framework to add support for a CLI chatbot Test environment and I want to update all dependencies to use the latest/stable react versions

@marktopper marktopper mentioned this pull request Jan 18, 2018
Closed
@marktopper
Copy link

@clue, any status on this?

@clue
Copy link
Owner

clue commented Jan 19, 2018

@marktopper, @WyriHaximus did a very good job at updating dependencies with this PR, but this has been superseded by changes on the master in the meantime. I've started picking this up again and will update the status in #50 as soon as time permits 👍

@WyriHaximus
Copy link
Author

Cool so closing this in that case 👍 🎉

@clue clue mentioned this pull request Jan 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
BC break maintenance new feature
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@WyriHaximus @clue @mpociot @marktopper