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

Mark all classes as final to discourage inheritance #134

Merged
merged 1 commit into from Jul 11, 2019

Conversation

Projects
None yet
4 participants
@WyriHaximus
Copy link
Member

commented Jul 11, 2019

Classes should be used via composition rather than extension.
This reduces our API footprint and avoids future BC breaks by avoiding
exposing its internal assumptions.

@WyriHaximus WyriHaximus added this to the v1.0.0 milestone Jul 11, 2019

@WyriHaximus WyriHaximus requested review from jsor and clue Jul 11, 2019

Show resolved Hide resolved src/Config/HostsFile.php Outdated
Show resolved Hide resolved src/Config/HostsFile.php Outdated
Show resolved Hide resolved src/Config/HostsFile.php Outdated
Show resolved Hide resolved src/Protocol/BinaryDumper.php Outdated

@WyriHaximus WyriHaximus force-pushed the WyriHaximus-secret-labs:final branch 3 times, most recently from 3e302e5 to 2c2c836 Jul 11, 2019

@WyriHaximus WyriHaximus force-pushed the WyriHaximus-secret-labs:final branch from 2c2c836 to eae3215 Jul 11, 2019

@clue

clue approved these changes Jul 11, 2019

@jsor

jsor approved these changes Jul 11, 2019

@jsor jsor merged commit f8a443b into reactphp:master Jul 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@WyriHaximus WyriHaximus deleted the WyriHaximus-secret-labs:final branch Jul 11, 2019

@webdevmatics

This comment has been minimized.

Copy link

commented on eae3215 Jul 17, 2019

You have to test code before committing. we are getting this issue beyondcode/laravel-websockets#216

This comment has been minimized.

Copy link
Member Author

replied Jul 17, 2019

a) We did, but we simply can't just test all dependants out there, especially when it's not listed on https://packagist.org/packages/react/dns/dependents because react/dns is an indirect dependency.
b) This is a major release which allows for BC breaks.

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.