php-fig / container

MIT License
9.93k stars 52 forks source link

Prepare version 2.0.0 of container interface with enabled strict types. #19

Closed lisachenko closed 3 years ago

lisachenko commented 6 years ago

I want to propose to start discussion about moving the version to the next major with enabled typehints, so all new libraries and frameworks could rely on more stable and type-hinted interface.

sergeyklay commented 6 years ago

Could you please provide an example shows the issue which you want to solve?

lisachenko commented 6 years ago

Actually, there is no any issue, just want to make interface more strict for version 2.0. And there are two ways of doing this:

  1. Add strict types declaration and argument type hints, requiring PHP>=7.2. This will make this interface BC compatible with all current implementations thanks to the parameter type widening feature https://wiki.php.net/rfc/parameter-no-type-variance
    
    declare(strict_types=1);

interface ContainerInterface { public function get(string $id); public function has(string $id): }



2. Also add `bool` return type to the `has(string $id)` method to guarantee that nothing else could be returned from this method call. Unfortunately, this change requires breaking BC and bumping the major version of package.

Reason, why I post this issue is to avoid non-standard values for service identifiers for modern codebase.
moufmouf commented 6 years ago

I'm completely :+1: with step 1 (because we can do it with a minor version bump and specifying PHP >= 7.2 in the composer.json, thanks to parameter type widening)

However, I don't feel like the value added by a single return type for has justifies the major version bump. By doing a major version bump on a PHP-FIG package, you are really splitting all implementations and consumers in 2 groups, for really no practical value (There are not hundreds of containers out there and you can trust their author for returning a boolean) I mean: the interest of the argument type hint seems superior to the interest of the return type-hint.

mnapoli commented 6 years ago

Completely agree with you @moufmouf.

lisachenko commented 6 years ago

I'm also looking for the standard way to store services in the container and current specification lacks of set method or even advanced logic with definitions, etc... This can be good step towards next major version.

Jean85 commented 6 years ago

The set part was left out intentionally, see the meta doc. It should be covered by a new, separated PSR.

moufmouf commented 6 years ago

@Jean85 :+1: @lisachenko there is work in progress regarding putting things into containers. You can view the "interop" project here: https://github.com/container-interop/service-provider/

Jean85 commented 4 years ago

We've been discussing this kind of issues a lot lately, and we've condensed our proposed solution to updating PSR interfaces in a blog post. Please read it and provide your feedback!

https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/

mindplay-dk commented 3 years ago

Yeah, this would work. 👍

I've been upgrading my packages to require 7.2 or 7.3 minimum, as this appears in most cases to be the most feasible option when adding PHP 8 support - I was grooming my packages for missing type-hints in the process, and wanted to add a string type to these method implementations, but that's rejected since string would be a "narrowing" the untyped parameters.

In terms of versioning, if the package "requires": { "php": ">=7.2" }, it should be correctly versioned as 1.0.1 - this isn't a functional change, and the package would only be installable on PHP versions where this change is either harmless or useful.

Note that versioning this as 2.0.0 wouldn't work, since packages that currently depend on ^1.0 wouldn't be able to upgrade - on the other hand, if some dependencies require ^1.0 and others ^1.0.1, it'll resolve to 1.0.1, and be installable on PHP >= 7.2 only, hence, no problem.

However, packages that wish to type-hint their implementations must require version ^1.0.1 of this package.

This should probably be covered in the README, since some people won't think to do this - they may put ^1.0.0 in their requirements, which may resolve to 1.0.1 on their platform, and maybe their IDE suggests an auto-completion with the type-hint, or they simply notice it in the file and add it, not knowing the revision history.

It's pretty important we document this, or someone will inadvertently end up setting booby traps. 💣

Other than that, I think, yes, let's do this? 🙂