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

Add resource typehint and return type #1631

Open
wants to merge 1 commit into
base: master
from

Conversation

7 participants
@DASPRiD
Contributor

DASPRiD commented Nov 11, 2015

This PR adds a resource typehint and return type. Corresponding RFC:
https://wiki.php.net/rfc/resource_typehint

@laruence laruence added the RFC label Nov 29, 2015

@Fleshgrinder

This comment has been minimized.

Show comment
Hide comment
@Fleshgrinder

Fleshgrinder Jun 27, 2016

Contributor

@DASPRiD: is this abandoned?

Contributor

Fleshgrinder commented Jun 27, 2016

@DASPRiD: is this abandoned?

@DASPRiD

This comment has been minimized.

Show comment
Hide comment
@DASPRiD

DASPRiD Jun 27, 2016

Contributor

@Fleshgrinder it seems like an unwanted feature, since resource itself is too generic.

Contributor

DASPRiD commented Jun 27, 2016

@Fleshgrinder it seems like an unwanted feature, since resource itself is too generic.

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Jun 27, 2016

Contributor

As far as I remember is the type "resource" itself only a workaround from the time where PHP did not support proper object orientation. It remains in the php-src base only for BC reasons.

newer apis like e.g, PDO wrap this "handles" in a object and hide it from the user, which is the far more elegant approach.

Contributor

staabm commented Jun 27, 2016

As far as I remember is the type "resource" itself only a workaround from the time where PHP did not support proper object orientation. It remains in the php-src base only for BC reasons.

newer apis like e.g, PDO wrap this "handles" in a object and hide it from the user, which is the far more elegant approach.

@Fleshgrinder

This comment has been minimized.

Show comment
Hide comment
@Fleshgrinder

Fleshgrinder Jun 27, 2016

Contributor

That is all fine but there are still many APIs left that actually give you a resource and one might want to type hint against those, even if it is only internally while creating such a wrapping object. Of course, one cannot check the type of the resource itself via type hints, that is actually a problem.

Complicated situation … simply ignoring it is not a good approach imho.

Contributor

Fleshgrinder commented Jun 27, 2016

That is all fine but there are still many APIs left that actually give you a resource and one might want to type hint against those, even if it is only internally while creating such a wrapping object. Of course, one cannot check the type of the resource itself via type hints, that is actually a problem.

Complicated situation … simply ignoring it is not a good approach imho.

@DASPRiD

This comment has been minimized.

Show comment
Hide comment
@DASPRiD

DASPRiD Jun 27, 2016

Contributor

At least for file access, we have SplFileObject.

Contributor

DASPRiD commented Jun 27, 2016

At least for file access, we have SplFileObject.

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Jun 27, 2016

Contributor

typehint against resource itself is not very helpful either, because you still can pass e.g. a network connection handle into a function which expects a GD resource etc.

Contributor

staabm commented Jun 27, 2016

typehint against resource itself is not very helpful either, because you still can pass e.g. a network connection handle into a function which expects a GD resource etc.

@HallofFamer

This comment has been minimized.

Show comment
Hide comment
@HallofFamer

HallofFamer Jun 27, 2016

Well if in modern PHP era you still actually use resource type and pass it around functions/methods, then you aint following good OOP practices.

With files and databases you have SplFileObject/MySQLi/PDO classes that you can use. With other resource types such as CURL it wont hurt to write a wrapper class for it.

To summarize, there's no need to typehint on resource because theres no need to pass resource as a parameter/argument to methods/functions.

HallofFamer commented Jun 27, 2016

Well if in modern PHP era you still actually use resource type and pass it around functions/methods, then you aint following good OOP practices.

With files and databases you have SplFileObject/MySQLi/PDO classes that you can use. With other resource types such as CURL it wont hurt to write a wrapper class for it.

To summarize, there's no need to typehint on resource because theres no need to pass resource as a parameter/argument to methods/functions.

@Fleshgrinder

This comment has been minimized.

Show comment
Hide comment
@Fleshgrinder

Fleshgrinder Jun 27, 2016

Contributor

Unless you type hint your privates too. Something that is particularly useful in an environment with many developers who all change code, come and leave, … You are approaching the issue from the mindset of an open source developer I guess.

@staabm: that is exactly the problem I am seeing too. One would need something to specify the resource type too (e.g. function fn(resource<stream> $rs)) but that would still not solve the whole problem. My personal approach, as always as a very defensive programmer, is assert. 😋

Contributor

Fleshgrinder commented Jun 27, 2016

Unless you type hint your privates too. Something that is particularly useful in an environment with many developers who all change code, come and leave, … You are approaching the issue from the mindset of an open source developer I guess.

@staabm: that is exactly the problem I am seeing too. One would need something to specify the resource type too (e.g. function fn(resource<stream> $rs)) but that would still not solve the whole problem. My personal approach, as always as a very defensive programmer, is assert. 😋

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Jun 27, 2016

Member

Another issue that was brought up on the ML is that this will complicate migration away from resources, as we did with GMP for example. It would increase the amount of code that would have to change if a resource changes to an object. (Or we'd have to special-case the resource typehint to allow certain objects, which is it's own pit of vipers.)

Member

nikic commented Jun 27, 2016

Another issue that was brought up on the ML is that this will complicate migration away from resources, as we did with GMP for example. It would increase the amount of code that would have to change if a resource changes to an object. (Or we'd have to special-case the resource typehint to allow certain objects, which is it's own pit of vipers.)

@Fleshgrinder

This comment has been minimized.

Show comment
Hide comment
@Fleshgrinder

Fleshgrinder Jun 27, 2016

Contributor

We definitely should come up with a roadmap here and I will invest time if nobody else does before I find the time to do so.

We should most probably close this PR and mark + move the RFC to Withdrawn.

Contributor

Fleshgrinder commented Jun 27, 2016

We definitely should come up with a roadmap here and I will invest time if nobody else does before I find the time to do so.

We should most probably close this PR and mark + move the RFC to Withdrawn.

@s3b4stian

This comment has been minimized.

Show comment
Hide comment
@s3b4stian

s3b4stian Jul 30, 2017

Hello, I think that resource type hint and return type useful. With Psr-7 I would have liked this feature, precisely when i implemented the stream class.

I so wrote the constructor the stream class:

public function __construct($resource)
{
    if (!is_resource($resource)) {
        throw new InvalidArgumentException(__CLASS__.': Invalid resource provided');
    }

    if  ('stream' !== get_resource_type($resource)) {
        throw new InvalidArgumentException(__CLASS__.': Resource provided is not a stream');
    }

    $this->resource = $resource;
    $this->isPipe = $this->checkFileMode($resource);
}

With resource type hint, I would have written so:

public function __construct(resource $resource)
{
    //only check the type of resource
    if  ('stream' !== get_resource_type($resource)) {
        throw new InvalidArgumentException(__CLASS__.': Resource provided is not a stream');
    }

    $this->resource = $resource;
    $this->isPipe = $this->checkFileMode($resource);
}

Modifies in the php code are very short. I also forked the php-src repository and added myself the resource type hint at s3b4stian/php-src

Before begin a RFC I checked the requests already present and I found this.

Please reconsider the implementation.

s3b4stian commented Jul 30, 2017

Hello, I think that resource type hint and return type useful. With Psr-7 I would have liked this feature, precisely when i implemented the stream class.

I so wrote the constructor the stream class:

public function __construct($resource)
{
    if (!is_resource($resource)) {
        throw new InvalidArgumentException(__CLASS__.': Invalid resource provided');
    }

    if  ('stream' !== get_resource_type($resource)) {
        throw new InvalidArgumentException(__CLASS__.': Resource provided is not a stream');
    }

    $this->resource = $resource;
    $this->isPipe = $this->checkFileMode($resource);
}

With resource type hint, I would have written so:

public function __construct(resource $resource)
{
    //only check the type of resource
    if  ('stream' !== get_resource_type($resource)) {
        throw new InvalidArgumentException(__CLASS__.': Resource provided is not a stream');
    }

    $this->resource = $resource;
    $this->isPipe = $this->checkFileMode($resource);
}

Modifies in the php code are very short. I also forked the php-src repository and added myself the resource type hint at s3b4stian/php-src

Before begin a RFC I checked the requests already present and I found this.

Please reconsider the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment