-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Adding object type-hint and return-type #2080
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
Conversation
I say no to this, dont like adding object to reserved name. I use Object as root class for all my classes, it breaks my application completely. I think object can be used as parameter type hint already, or you can just use StdClass as typehint, so this is totally unnecessary either. |
@HallofFamer, you should use the mailing lists once this RFC reaches discussion stage instead of here |
@HallofFamer Root class was discussed with this feature but was put away for another discussion. It'll be discussed with another RFC after this one. For now it'wasn't discussed yet at internals mailing lists so far. |
So... how does type-hinting it to stdObject make it catch custom objects? Moreover, how's breaking your application an argument against this RFC? Since yours is a high-level class, it wouldn't that hard to refactor it, right? Other arguments should be brought up, not our specific issues. |
@HallofFamer having a root class which for eg acts exactly as |
@brzuchal In PHP though, objects lack a root class and has no properties/methods by default. To me, a method that accepts/returns object type doesnt tell me exactly what it is accepting/returning, I use an interface or abstract class type for hinting if I want to use type hinting feature. In your example of ORM, just dont specify return type, it is optional anyway. Like I said before, you use type hinting when the type is specific, rather than a generic object. Same reason why there is no need for typehinting on resource, because without knowing its specific type of object or resource its not useful at all. So yeah, my point stands that, unless a root object class is introduced with some methods(such as gethashCode(), which can be just like spl_object_hash()) that can be used on every object, having object type hinting is unnecessary. |
@HallofFamer you're missing the point of |
@brzuchal In those edge cases as you mentioned, just dont use type hinting at all, if there is no benefit of type hinting, really. In your defense, it may be a better idea to introduce a root class, but before a root class is implemented I dont see type hinting with object is useful. I am against reserving object as keyword also, because it just breaks backward compatibility with something totally unnecessary, especially when it is already PHP 7.1 and 7.2. |
@HallofFamer |
@brzuchal |
@HallofFamer I hate to break this to you but the world doesn't revolve around you, there are many benefits to this change as pointed out in the rfc, the automatic type validation is an interest of mine. Ongoing refactoring is an improve part of a programmer's job and I don't think it is asking much to do a simple rename, I did it, went from |
There is no need to break existing code. This stuff can be done in userland. To type hint against Object, create an interface Object an implement it to all your classes. |
@jerrygrey Maybe PHP can make StdClass the root class for all classes, thus solving this problem completely. But this will require another RFC, and I am not sure how likely it will be done. Or just as @sirsnyder said, create a root class or interface that all your classes inherit, like I did with Object class, and problem solved. Also soft reserved keyword doesnt mean anything, its not reserved after all. |
Generic object typehint could be useful in many ways, look e.g. at Doctrine ORM, a lot of public OO API deals with arbitrary/generic objects. Another example are serialization libraries like jms/serializer. The primary benefit of this typehint is in libraries imho. Also note that
Not really across multiple libraries / code bases. Maybe PSR could be a way, but support on language level is much better way.
That doesn't make much sense. stdClass is meant to be used as an object to store arbitrary data, pretty much like object variant of the array. |
Nope it is not useful in any ways, except that it tells you that it is an object of unknown class, which cannot even be used to enforce polymorphism. You are better off just not to use type hinting at all, if type hinting doesnt really tell you anything useful. The better way is to define an object root class that every PHP class implicitly extends to, the stdclass recommendation is just one of the several variants I can think of, which is not meant to be the ideal solution anyway. Object is the root class for essentially every programming languages that support OOP, with some exceptions such as ObjectiveC. It fits the mental model of a programmer that the root class is named Object, reserving the keyword object makes it impossible for userland implementation of Object root class. I've mentioned before, refactoring isnt a problem, but refactoring that is completely unnecessary and reduces productivity isnt the kind of refactoring we need. Of course, there are other ways to solve the problem, such as making PHP keyword case-sensitive, but it will result in too much BC break that is pretty much impossible now for this programming language. Alternatively, object type hinting will not cause 'object' to be reserved, instead the word 'object' resolves to Object class if defined, or object type if undefined. There are many ways to achieve this, BC break for the sake of BC break, doesnt sound like a good idea to me. |
I've prepared also version with root-class for POC and some tests and actually it slows down about 5% at object instantation if all clases inherits from |
@sirsnyder actually your are unable to do that in userland! You can implement |
Well slowing down by 5% is negligible, from statistical point of view it may not even be valid, and no one is going to make a deal for performance unless its at least 10%-15% slowdown. Ideally the root object class isnt just an empty object sitting there for type hinting purpose, it should have some useful methods such as hashCode(), getClass(). And that's exactly why it is more useful than your object type hinting, because now at the very least, you can be sure that every object has some methods you can call. For your object type hinting, it only tells the variable is an object. Nothing more, nothing less, and nothing really helpful. And what does Java have anything to do in this discussion? Having a root Object class is not making PHP look like Java, it makes PHP a more complete OO language. Almost every OO language has Object as root class name, even javascript. PHP is like jack of all trades, it brings in whatever is useful, from any existing programming languages. PHP's base OO model looks like Java's, namespaces looks like C++/C#'s, closures looks like javascript's, trait looks like scala's, generator looks like python/ruby's. See the point? Also soft-reserved keyword doesnt mean anything, it is not reserved and I can use it anyway. |
@HallofFamer sure I see the point you can post it into php-internals mailing list, this RFC deosn't fit your needs and that's all. World doesnt revolve around you and your broken implementation and I'm not goint to convince you anymore. |
The world doesnt revolve around you either, it revolves around nobody, which is why you are making this RFC and I am arguing against it. You have you rights to make it, I have my rights to go against it. I am not trying to convince you either, since apparently you cannot be convinced and are determined to make breaking changes for little benefit. |
It seems to be segfaulting on Travis: https://travis-ci.org/php/php-src/jobs/170057961#L536 |
A wonderful addition for DI especially, and for those (such as myself) who prefer to use objects as their |
Zend/zend_execute.c
Outdated
@@ -925,6 +928,8 @@ static zend_always_inline zend_bool zend_check_type( | |||
} | |||
} | |||
return 0; | |||
// } else if (arg_info->type_hint == IS_OBJECT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left overs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, it's okay now.
@brzuchal Is variance in typehint working for all kind of object inheritance or just with "object" keyword ? Maybe it's out of the scope of this PR but I think it's important to keep in mind it for consistency. <?php
class Animal {
public function test() : Animal {}
}
class Dog extends Animal {
public function getClone() : Dog {
return clone $this;
}
}
$dog = new Dog();
$dog->getClone(); |
@raziel057 It only works for |
@brzuchal Ok thanks for your quick answer and for your hard work! |
Hi @brzuchal, do you plan to go forward with this and move it to the voting phase so it could eventually make it to PHP 7.2? |
Agree: very needed addition 👍 |
I've changed my remote branch to version before variance patch. But I don't know how to trigger builds on this PR. Can someone help please? |
@brzuchal You need to rebase the PR to current master. The CI build needs to be able to merge into master without conflicts. |
@nikic I think I did it. My changes are passing AppVeyor and Travis with ZTS and debug enabled, don't know why build without ZTS and debug has failed, but I can assume this is because of unrelated changes - because all object type tests passed. I also removed tests which were checking argument type inheritance because of Parameter type widening which was accepted and implemented already in 7.2. |
@sgolemon could you please review and merge? |
} | ||
|
||
--EXPECTF-- | ||
Fatal error: Interface function One::a() cannot contain body in %s on line 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not what you want to test here ;)
Stack trace: | ||
#0 %s(12): class@anonymous->a() | ||
#1 {main} | ||
thrown in %s on line 9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test looks redundant with Zend/tests/object_types/return_type_in_class.phpt, which also has an object type on the interface.
Can you please add a test for |
Merged as 8e10c9d with the remaining issues fixed. Thanks! |
@nikic Thank you very much. I hadn't enough time to fix issues. |
🍻
…On 26 Jun 2017 12:21 PM, "Bogdan Padalko" ***@***.***> wrote:
@brzuchal <https://github.com/brzuchal>, @nikic <https://github.com/nikic>
and all contributors: thank you for making this possible, for your time and
efforts!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2080 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAJakJz6lkTmuDmj3KC7qWuB94NmX2TDks5sH4YegaJpZM4JjsIq>
.
|
`object` was already a soft reserved keyword as of PHP 7.0. As of PHP 7.2, it: * can be used as a parameter type hint * can be used as a return type hint * has become a "hard" reserved keyword. References: * https://wiki.php.net/rfc/object-typehint * php/php-src#2080
This patch is for PHP RFC: Object typehint.
This patch enable and adjust the code, add phpt.