Skip to content

Replace is_callable checks with type hints #16125

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

Merged
merged 2 commits into from
Oct 6, 2015

Conversation

nicolas-grekas
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #14330
License MIT
Doc PR -

Also removes tests checking the exceptions thrown from
the removed is_callable checks.

Also removes tests checking the exceptions thrown from
the removed is_callable checks.
@Tobion
Copy link
Contributor

Tobion commented Oct 5, 2015

This is the same BC break as in ##14330. You get a warning for overwritten method that the signature is not the same: https://3v4l.org/ruPcK

But I'm 👍 to add callable typehints.

@stof
Copy link
Member

stof commented Oct 5, 2015

@Tobion in this PR, it is OK:

  • constructors don't need to have a matching signature
  • most other classes are not meant to be extended (the only case I see is Command, and you won't override setCode in such case but execute)

@stof
Copy link
Member

stof commented Oct 5, 2015

@nicolas-grekas HttpKernelTest::testHandleWhenTheControllerIsNotACallable is broken here

and it would be great to fix the deps=low tests of the Translation component

@nicolas-grekas
Copy link
Member Author

I added more callable type hints that should all qualify as constructors or methods not meant to be extended.
@stof I'll look at deps=low tests if you look at #16097 ;-)

*/
public function __construct($caster, \Exception $prev)
public function __construct(\Exception $prev)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be changed in older branches actually ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, see #16129

@Tobion
Copy link
Contributor

Tobion commented Oct 5, 2015

👍

Status: Reviewed

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 7685cdd into symfony:master Oct 6, 2015
fabpot added a commit that referenced this pull request Oct 6, 2015
…nicolas-grekas)

This PR was merged into the 3.0-dev branch.

Discussion
----------

Replace is_callable checks with type hints

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |  #14330
| License       | MIT
| Doc PR        | -

Also removes tests checking the exceptions thrown from
the removed is_callable checks.

Commits
-------

7685cdd Add more callable type hints
4e0c6e1 Replace is_callable checks with type hints
@nicolas-grekas nicolas-grekas deleted the typehint-callable branch October 7, 2015 06:26
@fabpot fabpot mentioned this pull request Nov 16, 2015
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.

6 participants