Adding type-hints and extending Throwable for exception interfaces #20

moufmouf · 2018-06-26T15:57:46Z

Starting with PHP 7.2, parameter type widening allows us to omit type-hint in implementations.

This gives us a unique opportunity: we are now able to create a ContainerInterface that has scalar type-hints while retaining compatibility with all implementations. Since usage of scalar type-hints is one of the most requested features of PSR-11, we can fulfill that demand.

We can do this only from PHP 7.2. This PR proposes to:

bump package version to 1.1
bump minimum PHP version to 7.2
add type hints to get and has

Composer will do its job and solve dependencies by itself. If PHP < 7.2 is used, it will install psr/container 1.0.x. If PHP 7.2+ is used, it will bump the version to psr/container 1.1.x.

Users running composer with PHP 7.1 and composer update --ignore-platform-reqs (I know some!) will certainly have a surprise, so we might want to write a blog post somewhere explaining what to do before merging this.

As a side effect, since we now are using PHP 7, we can rewrite the ContainerExceptionInterface to extend Throwable. This is a best practice as it prevents using this interface on anything but an Exception.

This PR does not add a "bool" return type on has as this would introduce a compatibility break that is not worth bumping to a major version (this would split the PHP ecosystem in 2 and we don't want to do that).

Closes #19
Closes #12

mnapoli · 2018-06-26T17:11:37Z

This is great, however as you mentioned it will cause problems for:

end users running PHP <7.1 and running composer update --ignore-platform-reqs
end users running PHP <7.1 in production and PHP >=7.1 in development

Yes both those users are "wrong". The first should probably not run that, and the second should use:

"config": {
    "platform": {
        "php": "their prod version"
    }
},

But I'm afraid it might affect many end users?

Update: I now agree that this change should happen

Ocramius · 2018-06-26T20:35:08Z

composer update --ignore-platform-reqs

is pretty much synonymous to "I'm here and I'm asking for trouble", and should not be considered

end users running PHP <7.1 in production and PHP >=7.1 in development

Also here: people playing with fire. Please don't feel scared of upgrading things as they should be: https://www.doctrine-project.org/2017/07/25/php-7.1-requirement-and-composer.html

odan · 2018-06-26T21:38:10Z

src/ContainerInterface.php

     * @return bool
     */
-    public function has($id);
+    public function has(string $id);


Would it be possible to add :bool as return type? Example:

public function has(string $id): bool;

This question is addressed in the last paragraph :)

Ah thanks :-) I think this is handled on the basis of the new PHP version 7.2 but ok.

moufmouf · 2018-06-27T07:23:26Z

Also here: people playing with fire. Please don't feel scared of upgrading things as they should be: https://www.doctrine-project.org/2017/07/25/php-7.1-requirement-and-composer.html

Pretty cool.

I tend to agree with @Ocramius, but I also can understand the fact that big framework authors don't want to upset their user base (even the user is not playing by the book).
@Ocramius : when you introduced those changes, was there a lot of troubles among the Doctrine community or did it ran smoothly?

Also, I just read the Symfony BC promise and I realize we are doing a change that goes against their promise:

See https://symfony.com/doc/current/contributing/code/bc.html

Type of Change	Change Allowed
...	...
Add type hint to an argument	No
...	...

Now, I also understand that promise was written before PHP 7.2 came out so it might no longer apply, as this is no longer relevant.

Anyway, we should still ping the big players out there and ask for their advice.

So here we go.
Ping @nicolas-grekas @fabpot @taylorotwell

Ocramius · 2018-06-27T12:48:20Z

@Ocramius : when you introduced those changes, was there a lot of troubles among the Doctrine community or did it ran smoothly?

Yes, there was some noise, which is why the article got written. I expect some rage again, but it's usually from developers that:

didn't figure out what composer.lock is for (they should)
run dev/prod with different environments (again: requires CI that is close to prod)

Therefore be courageous and just do it.

Be aware that the ZF community has been doing this without problems for a lot of time (mostly because of SAT awareness), and Doctrine started doing these bumps (and will keep doing them) regularly in minor releases.

nicolas-grekas · 2018-06-27T12:52:07Z

If the PHP minimal requirement is bumped to php 7.2, I suppose adding the arguments is fine. The BC policy basically lists things that break existing code. On 7.2, the rule for arguments changed so the BC policy will align (and we'll thus be able to add type hints in Symfony 5.)

Ocramius · 2018-06-27T12:55:00Z

Is there something that can be added to make this crash eagerly if run on PHP <= 7.1? That would make things more explicit.

Jean85 · 2018-06-27T13:05:48Z

Is there something that can be added to make this crash eagerly if run on PHP <= 7.1? That would make things more explicit.

Apart from a very ugly if (PHP_VERSION...) at the top of the files, I fear not. The biggest syntax differences are object and void, and we can't use them without causing breaking changes...

Crell · 2018-06-27T15:23:08Z

I hate to be a party pooper, but this would be a spec change and thus require a new PSR.

Yes, it's mostly-BC changes, but it's still a spec change.

Ocramius · 2018-06-27T15:27:49Z

Yeah, makes sense tbh - unsure how the packagist naming would work though, since the container PSR is already taken.

moufmouf · 2018-06-27T19:52:27Z

I hate to be a party pooper, but this would be a spec change and thus require a new PSR.

Oh noes! I guess you are right, the PSR-11 spec contains the code of the interfaces so it seems legit to create a new PSR to update this part.

Yeah, makes sense tbh - unsure how the packagist naming would work though, since the container PSR is already taken.

@Ocramius , I don't think I understand. I'm not sure this requires another package. What prevents us from deprecating PSR-11, creating a new updated PSR-19 that still targets the same package (psr/container)?

TBH, I'm not sure I have the strengh to go through all the PSR process just to add type-hints... but theoretically, is it ok to keep the psr/container package for an updated PSR? I'd say yes. What do you think @Crell ?

nicolas-grekas · 2018-06-27T20:04:55Z

Why deprecate ? Just bump the version to 1.1. I agree with @Crell this needs some formal process, that's a good opportunity to learn how to do this for the group. I don't think this should follow the same process as new RFCs. Don't you think?

moufmouf · 2018-06-27T20:08:26Z

I agree with @Crell this needs some formal process, that's a good opportunity to learn how to do this for the group. I don't think this should follow the same process as new RFCs. Don't you think?

👍

I would absolutely love not to have to go through the new RFC process. But I'm not sure the current bylaws have a process for this kind of changes. I'll bring the issue to the mailing list.

Crell · 2018-06-27T23:19:47Z

Currently there is no process defined for this, as it hasn't come up except as "a thing we should do at some point". We should discuss it on the mailing list as there's a not small number of variables to consider. It's come up a few times in the past as a hypothetical but no one has been really interested in addressing it.

This relates to any non-trivial updates to specs, and if we should handle them differently. Eg, adding type hints vs folding in errata vs correcting bugs in ways that would be an API change from the previous spec. We should look at the situation holistically.

Personally I'd be fine with a 1.1 or 2.x version of the package to represent an update to an existing spec or a supplanting PSR; the question is what the approval process is for those and how we differentiate a 1.1 vs 2.0 change.

Jean85

I've opened a discussion on the ML about this kind of stuff: https://groups.google.com/forum/#!topic/php-fig/JqHnOw_8R1s

This PR should be on hold until that is cleared out.

Jean85 · 2019-10-04T14:08:09Z

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/

weierophinney

🚢

ashnazg

Wait... aren't these changes already merged in via PR #27 ?

donatorsky · 2021-01-25T19:49:08Z

Hi there! PR seems to be a bit outdated (2 years, huh...)

This PR does not add a "bool" return type on has as this would introduce a compatibility break that is not worth bumping to a major version (this would split the PHP ecosystem in 2 and we don't want to do that).

Do You think it is worth bumping a major version with PHP 8.x in mind? This could potentially go together with the following:

public function get(string $id): mixed;

ashnazg · 2021-01-25T19:56:47Z

This PR does not add a "bool" return type on has as this would introduce a compatibility break that is not worth bumping to a major version (this would split the PHP ecosystem in 2 and we don't want to do that).

Do You think it is worth bumping a major version with PHP 8.x in mind? This could potentially go together with the following:
public function get(string $id): mixed;

This is discussed in #28 ... it can't be done in the same step as this PR.

derrabus

Let's ship this, please.

derrabus · 2021-03-01T00:10:19Z

composer.json

    "extra": {
        "branch-alias": {
-            "dev-master": "1.0.x-dev"
+            "dev-master": "1.1.x-dev"


Suggested change

"dev-master": "1.1.x-dev"

"dev-master": "2.0.x-dev"

Following this blog article, this change would mean a bump to 2.0.

No it doesn't. This PR doesn't add return types so, by requiring PHP 7.2, it's allowing a smooth upgrade path and a minor version is ok. #28 is instead adding return types and hence requiring a major bump.

Vote is already in progress for the approval of these two new versions, and it looks like it's passing: https://groups.google.com/g/php-fig/c/niYwPGatoz4

This PR doesn't add return types so, by requiring PHP 7.2, it's allowing a smooth upgrade path and a minor version is ok.

I'm not arguing with that. It's just not what has been announced in the blog article I've linked. If your processed has changed since then, that article should probably be updated.

A newer blog post was published the month after: https://www.php-fig.org/blog/2019/12/new-bylaw-and-next-election-cycle/
And it's referring to the approval of a PHP-FIG bylaw that these PR are following: https://www.php-fig.org/bylaws/psr-evolution/

mynameisbogdan · 2021-03-05T16:27:12Z

src/ContainerExceptionInterface.php

 * Base interface representing a generic exception in a container.
 */
-interface ContainerExceptionInterface
+interface ContainerExceptionInterface extends \Throwable


This change breaks a lot of symfony installations.

Seems fixed since PHP 7.4.0+

https://3v4l.org/vUsLI

You're correct, but 7.3 it's not yet EOL.

And thank you for the link, I wasn't aware of that change in 7.4.

As a BC change I wonder why it wasn't shipped with 2.0.0.

Honestly, we missed this; it's a language bug that we weren't expecting. We will probably reintroduce the change bumping the PHP requirement to 7.4, see #32

mindplay-dk · 2021-03-07T15:54:06Z

I had to do a bit of thinking to understand why adding a return-type wouldn't work.

To others trying to understand this - look:

<?php

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

class Container implements ContainerInterface
{
    public function get($id) // incompatible! implicitly returns `mixed`
    {
        // ...
    }
}

The first thing to understand is that functions with no return-type implicitly return mixed, not void - without a declared return-type, you can't tell what the function returns, so in terms of type-checking for variance, PHP has to assume it means mixed.

If we added a bool return type, that's a breaking change, because it's a narrowing of the implicit mixed return-type it had before. An existing implementation without the bool return-type implicitly returns mixed, which isn't compatible with bool - so even with the introduction of full co/contra-variance in PHP 7.4, this wouldn't work.

Adding type-hints and extending Throwable for exception interfaces

42ca75b

moufmouf requested review from Jean85, michaelcullum, mnapoli and weierophinney June 26, 2018 15:57

odan reviewed Jun 26, 2018

View reviewed changes

Jean85 requested changes Jun 28, 2018

View reviewed changes

hultberg mentioned this pull request Aug 25, 2018

ContainerExceptionInterface is not throwable #21

Closed

jstewmc mentioned this pull request Oct 9, 2018

Remove PSR 11 changes jstewmc/gravity#29

Merged

weierophinney approved these changes Jan 25, 2021

View reviewed changes

weierophinney mentioned this pull request Jan 25, 2021

Make ContainerExceptionInterface extend Throwable where it is available #23

Closed

ashnazg approved these changes Jan 25, 2021

View reviewed changes

ashnazg reviewed Jan 25, 2021

View reviewed changes

donatorsky approved these changes Jan 25, 2021

View reviewed changes

derrabus reviewed Mar 1, 2021

View reviewed changes

weierophinney merged commit 9fc7aab into php-fig:master Mar 5, 2021

mynameisbogdan reviewed Mar 5, 2021

View reviewed changes

mnapoli mentioned this pull request Mar 5, 2021

Fatal error with v1.1.0 and PHP 7.2 / 7.3 #30

Closed

This was referenced Mar 8, 2021

Bump psr/container from 1.0.0 to 1.1.1 browscap/browscap-site#127

Closed

Update psr/container requirement from ^1.0 to ^1.0 || ^2.0 antidot-framework/container#21

Closed

Bump psr/container from 1.0.0 to 1.1.1 lcobucci/error-handling-middleware#171

Merged

dependabot bot mentioned this pull request Mar 18, 2021

chore(deps): bump psr/container from 1.0.0 to 1.1.1 labor-digital/typo3-better-api#10

Closed

Jean85 mentioned this pull request Mar 31, 2022

Why dependency on PHP 7.4.0 #43

Closed

Adding type-hints and extending Throwable for exception interfaces #20

Adding type-hints and extending Throwable for exception interfaces #20

Conversation

moufmouf commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnapoli commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moufmouf commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius commented Jun 27, 2018

Uh oh!

nicolas-grekas commented Jun 27, 2018

Uh oh!

Ocramius commented Jun 27, 2018

Uh oh!

Jean85 commented Jun 27, 2018

Uh oh!

Crell commented Jun 27, 2018

Uh oh!

Ocramius commented Jun 27, 2018

Uh oh!

moufmouf commented Jun 27, 2018

Uh oh!

nicolas-grekas commented Jun 27, 2018

Uh oh!

moufmouf commented Jun 27, 2018

Uh oh!

Crell commented Jun 27, 2018

Uh oh!

Jean85 left a comment

Choose a reason for hiding this comment

Uh oh!

Jean85 commented Oct 4, 2019

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

ashnazg left a comment

Choose a reason for hiding this comment

Uh oh!

donatorsky commented Jan 25, 2021

Uh oh!

ashnazg commented Jan 25, 2021

Uh oh!

derrabus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mindplay-dk commented Mar 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

moufmouf commented Jun 26, 2018 •

edited

Loading

mnapoli commented Jun 26, 2018 •

edited

Loading

moufmouf commented Jun 27, 2018 •

edited

Loading