Skip to content

Request #75765 Throw exception instead of a fatal error when extends/implements #3010

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

Closed
wants to merge 1 commit into from

Conversation

timurib
Copy link
Contributor

@timurib timurib commented Jan 6, 2018

Extending an undefined class or implementing an undefined interface causes the fatal error instead of throwing Error exception. Relevant issue: https://bugs.php.net/bug.php?id=75765
This patch is attempt to fix it.

I'm not fully understand the status of current behavior: it is intentional or not. Most of fatal errors was replaced by exceptions, but some errors are still handling in "old manner". As said in the docs, a bugfix PR should be branched against the lowest actively supported branch. But I not sure about BC things, if someone depends on current error handling of this cases. Also the patch cannot be applied for previous versions "as is", because it affects some lines which was changed between versions. So far I've created a branch against master.

…from an undefined class or implementing an undefined interface.
@timurib timurib changed the title Bugfix-75765 Throw exception instead of a fatal error when extends/implements Bugfix #75765 Throw exception instead of a fatal error when extends/implements Jan 6, 2018
@timurib timurib changed the title Bugfix #75765 Throw exception instead of a fatal error when extends/implements Request #75765 Throw exception instead of a fatal error when extends/implements Jan 8, 2018
@timurib
Copy link
Contributor Author

timurib commented Jan 8, 2018

As nikic said, supporting this would require larger changes. I closed the PR, sorry.

@timurib timurib closed this Jan 8, 2018
@nikic
Copy link
Member

nikic commented Jan 8, 2018

If you like, we can land the extends part of this PR, because this is safe. The parent class is fetched prior to binding, so there is no issue there. We only run into problems with multi-phase binding for interface implementations and trait uses.

@timurib
Copy link
Contributor Author

timurib commented Jan 9, 2018

You are right, the implements is trickier:

try {
	class A implements B {}
} catch (Throwable $e) {
	var_dump(class_exists(A::class)); // Ouch! 
}

With extends this does not happen, but I decided that I missed some other case. Thanks for the explanation.

I will remove all about the interfaces and improve the tests and re-open the PR.

@timurib
Copy link
Contributor Author

timurib commented Jan 9, 2018

Looks like github prohibits to reopen a PR after force-push (I'm rebase & squash the branch).
Should I open a new PR?

@nikic
Copy link
Member

nikic commented Jan 9, 2018

Yeah, please do so.

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.

2 participants