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

Enforce Throwable rejection reason and require PHP 7+ as a consequence #138

Merged
merged 1 commit into from
May 9, 2019

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Apr 19, 2019

By enforcing Throwable as a rejection reason:

  • Get rid of all the double checks for Throwable and Exception
  • Can use typehints for it since we dropped direct Exception support (still supported through Throwable)
  • Bump to PHP 7.x+
  • Add note about support for older versions

src/RejectedPromise.php Outdated Show resolved Hide resolved
src/Exception/CompositeException.php Show resolved Hide resolved
@WyriHaximus WyriHaximus changed the title [WIP] Raise minimum PHP version to 7.0 [WIP] Enforce Throwable rejection reason and require PHP 7+ as a consequence Apr 20, 2019
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM 👍 I'll await @jsor's review first, but I don't see any major blockers.

I wonder if it makes sense to add test cases for incorrect usage, i.e. how an automatic TypeError for invalid arguments will bubble through common promise handlers etc.?

@WyriHaximus
Copy link
Member Author

I wonder if it makes sense to add test cases for incorrect usage, i.e. how an automatic TypeError for invalid arguments will bubble through common promise handlers etc.?

Makes sense, I'll add those 👍

@WyriHaximus
Copy link
Member Author

WyriHaximus commented Apr 22, 2019

@jsor now that everything must be a Throwable does the fatalError function still make sense?

@jsor
Copy link
Member

jsor commented Apr 23, 2019

now that everything must be a Throwable does the fatalError function still make sense?

Not sure what you mean?

@WyriHaximus
Copy link
Member Author

now that everything must be a Throwable does the fatalError function still make sense?

Not sure what you mean?

If you look at this failed test cause for example, this exception shouldn't have been thrown not triggered as an error: https://travis-ci.org/reactphp/promise/jobs/523187495#L499

@jsor
Copy link
Member

jsor commented Apr 23, 2019

@WyriHaximus In #97, we intentionally switched from throwing exceptions to aborting with a fatal error. Exceptions should (and after that patch actually do) never bubble out of the promise chain. You need to use the ErrorCollector in the tests, see eg. https://github.com/reactphp/promise/blob/master/tests/PromiseTest/PromiseFulfilledTestTrait.php#L219-L236

@WyriHaximus
Copy link
Member Author

@jsor ah awesome thanks 👍

@WyriHaximus
Copy link
Member Author

@jsor / @clue Ok added those tests.

@WyriHaximus WyriHaximus changed the title [WIP] Enforce Throwable rejection reason and require PHP 7+ as a consequence Enforce Throwable rejection reason and require PHP 7+ as a consequence Apr 26, 2019
@WyriHaximus
Copy link
Member Author

@clue / @jsor Added the note about supporting 2.0 and 1.0

README.md Outdated Show resolved Hide resolved
src/Exception/CompositeException.php Show resolved Hide resolved
tests/FunctionRejectTest.php Outdated Show resolved Hide resolved
tests/FunctionRejectTest.php Outdated Show resolved Hide resolved
Bump minimum required and tested PHP version to 7.0 as a result of
requiring Throwables. Renamed the nesecary exceptions, methods and
properties to match the new Throwable naming.
@WyriHaximus
Copy link
Member Author

@jsor @clue Processed the last comments and discussions. Also squashed everything into one commit.

@jsor jsor merged commit edd18a6 into reactphp:master May 9, 2019
@WyriHaximus
Copy link
Member Author

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

WyriHaximus added a commit to WyriHaximus-secret-labs/promise that referenced this pull request Oct 7, 2019
…consequence

With reactphp#138 requiring PHP 7.0 as a minimum we can now add return type
hints to all our public and private functions. To give all functions
return type hints we need the `void` return type, which isn't available
until PHP 7.1. So in order use that we also have to bump the the
minimum required PHP version for this package to PHP 7.1.
WyriHaximus added a commit to WyriHaximus-secret-labs/promise that referenced this pull request Oct 7, 2019
…consequence

With reactphp#138 requiring PHP 7.0 as a minimum we can now add return type
hints to all our public and private functions. To give all functions
return type hints we need the `void` return type, which isn't available
until PHP 7.1. So in order use that we also have to bump the the
minimum required PHP version for this package to PHP 7.1.

The benefit of return type hints is the ensurance at language level
of our return values. For ourselfs and our consumers.
WyriHaximus added a commit to WyriHaximus-secret-labs/promise that referenced this pull request Oct 7, 2019
…consequence

With reactphp#138 requiring PHP 7.0 as a minimum we can now add return type
hints to all our public and private functions. To give all functions
return type hints we need the `void` return type, which isn't available
until PHP 7.1. So in order use that we also have to bump the the
minimum required PHP version for this package to PHP 7.1.

The benefit of return type hints is the assurance at language level
of our return values. For ourself and our consumers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants