Skip to content

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Dec 14, 2018

When adding methods from a trait, we must not assume that a method name
with the same length as the name of the using class is either a PHP 4
style constructor, or not a magic method at all – it may well be
another magic method.

We preserve the spirit of the optimization which caused this
regression, and avoid string comparisons for all method names which can
never be magic methods.

When adding methods from a trait, we must not assume that a method name
with the same length as the name of the using class is either a PHP 4
style constructor, or not a magic method at all – it may well be
another magic method.

We preserve the spirit of the optimization which caused this
regression, and avoid string comparisons for all method names which can
never be magic methods.
@cmb69 cmb69 requested a review from nikic December 14, 2018 19:30
@nikic
Copy link
Member

nikic commented Dec 14, 2018

Fun case: What happens if the class is also called __Isset?

@nikic
Copy link
Member

nikic commented Dec 14, 2018

Answer: https://3v4l.org/0KjBR It's treated as a ctor...

}
zend_string_release_ex(lowercase_name, 0);
} else if (ZSTR_VAL(mname)[0] != '_' || ZSTR_VAL(mname)[1] != '_') {
if (ZSTR_LEN(ce->name) != ZSTR_LEN(mname) && (ZSTR_VAL(mname)[0] != '_' || ZSTR_VAL(mname)[1] != '_')) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to extract the checking logic into a separate function (length + name comparison) and keep the check in this location (rather than moving to the end).

@cmb69
Copy link
Member Author

cmb69 commented Dec 15, 2018

Answer: https://3v4l.org/0KjBR It's treated as a ctor...

Fascinating! However, it seems to me this example is not about zend_add_magic_methods(); something like https://3v4l.org/LIVrE is, and it shows that changing the order of the checks introduced a hypothetical BC break. Unless there is some good reason to change the order, I'd rather stick with the PHP-7.2 one, until we finally drop support for old-style constructors.

@nikic
Copy link
Member

nikic commented Dec 15, 2018

You're right, I was testing the wrong thing... In that case this patch LGTM.

@cmb69
Copy link
Member Author

cmb69 commented Dec 16, 2018

Thanks, @nikic! Applied as 0061db5.

@cmb69 cmb69 closed this Dec 16, 2018
@cmb69 cmb69 deleted the bug-77291 branch December 16, 2018 12:47
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