Allow throwing exception while loading parent class #4697
Conversation
45291fc to
91b0a0d
Compare
|
Awesome, thank you! I'll try this once its merged if you don't mind :) |
|
@dstogov Can you please take a look at this? I don't like doing this, but ability to throw exceptions for undefined parent/interface is important for Symfony to support their current implementation of optional dependencies, which they cannot easily remove. |
There was a problem hiding this comment.
I understood the incompatibility introduced in PHP-7.4 and agree it should be fixed.
Missed "parent" class should throw exception. I don't see a problem extending this to interfaces.
The patch slightly increase complexity of already extremely complex "inheritance + type & variance check" algorithm. It's OK to merge it, if it does everything right.
| Class A does not exist | ||
| Class A does not exist | ||
|
|
||
| Fatal error: During inheritance of B: Uncaught Exception: Class A does not exist in %s:%d |
There was a problem hiding this comment.
It's very unclear, why two times we got exceptions and now a fatal error.
| @@ -30,6 +30,8 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par | |||
| #define zend_do_inheritance(ce, parent_ce) \ | |||
| zend_do_inheritance_ex(ce, parent_ce, 0) | |||
|
|
|||
| /* TODO: Merge these in master -- only here to prevent ABI change. */ | |||
| ZEND_API int zend_do_link_class_ex(zend_class_entry *ce, zend_string *lc_parent_name); | |||
| ZEND_API void zend_do_link_class(zend_class_entry *ce, zend_string *lc_parent_name); | |||
There was a problem hiding this comment.
I think this ABI break is not a problem before 7.4 GA.
| @@ -3731,7 +3731,9 @@ static void preload_link(void) | |||
| } else { | |||
| CG(zend_lineno) = ce->info.user.line_start; | |||
| } | |||
| zend_do_link_class(ce, NULL); | |||
| if (zend_do_link_class_ex(ce, NULL) == FAILURE) { | |||
| ZEND_ASSERT(0 && "Class linking failed?"); | |||
There was a problem hiding this comment.
Is't possible to not preload just this class?
There was a problem hiding this comment.
Not easily, because then other classes also may not be preloaded because of that. The only way in which class linking can fail (without a fatal error I mean) is if parent/interface is missing. However, we have ensured beforehand that they exist, so this cannot happen, which is why this assertion is here.
|
Merged as 4b9ebd8 with _ex function removed and a slightly more explicit error message (so there is at least something to Google...) @nicolas-grekas After this change, I only have three remaining test failures on symfony: |
…-grekas) This PR was merged into the 3.4 branch. Discussion ---------- Re-enable previously failing PHP 7.4 test cases | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix #32995 | License | MIT | Doc PR | - The remaining PHP 7.4 issue has been fixed by Nikita in php/php-src#4697 This should be green once Travis updates their php7.4snapshot Commits ------- 9e4e191 Re-enable previously failing PHP 7.4 test cases
This is a fix for symfony/symfony#32995.
The behavior is:
cc @nicolas-grekas