Adds parameter typehints to ContainerInterface

[weierophinney]

This patch bumps the minimum supported PHP version to 7.2 and adds parameter typehints to ContainerInterface, as the first step towards adding explicit typehints based on the specification.

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

[weierophinney]

This patch will require creating a 2.0.01.1.0 release.

[Jean85]

@weierophinney as I said on the ML, couldn't we release this as 1.1? (and #28 as 2.0)
There's this exact example in the bylaw: https://www.php-fig.org/bylaws/psr-evolution/#psr-11-the-interface

[weierophinney]

Ooops! My bad! I'll update the milestone and description to mention 1.1 here, and update #28 as well.

[weierophinney]

Ping @moufmouf — are you okay with this?

[nicolas-grekas]

Thanks for moving this forward.

[mnapoli]

As this brings no bc-break, it's a no-brainer to merge.

@moufmouf ok with this?

Does anyone know if there is any protocol that we need to follow before merging and tagging? I'm up to do it, but don't want to make any mistake.

Again, unlike #28 there's no bc break so I think this one can be done asap.

[moufmouf]

👍 I'm definitely ok with this.

[mnapoli]

Maybe @Crell knows if we can merge this?

[Jean85]

@mnapoli @moufmouf the bylaw that you need to follow is this one: https://www.php-fig.org/bylaws/psr-evolution/
This PR follows on that exact upgrade path.

[mnapoli]

OK let's do this then, thanks a lot @Jean85 for clarifying this.

[mnapoli]

I'll tag a release tomorrow, just to give a bit more notice. (can you tell I am afraid of messing something up? 😄)

[Jean85]

@mnapoli PLEASE DO NOT TAG 😓 we need to pass a vote: https://www.php-fig.org/bylaws/psr-evolution/#workflow

Since releasing new versions of the interfaces MUST NOT alter the PSR in its behavior, those releases can be voted in with the same process as errata changes. The new releases MUST be declared and embedded with a brief explanation and a link in the PSR document, [...]

I'm happy to help you through this, but you need to collect all the work first: you already completed this PR, but we need to prepare (and not merge yet) #28 too, and prepare a PR for the bylaws that describe/includes all of this.

Then we can call the vote, wait for the standard two weeks discussion time, and then have the vote itself.
Sorry if it's a bit long, but that should help you with your fears 😄

[mnapoli]

@Jean85 ha good call, thanks!

OK so it seems @weierophinney did all the work already (I'm catching up with all repositories and the ML): php-fig/fig-standards#1215

So the next step is to notify the mailing list that there will be a vote in 2 weeks, is that correct?

[Jean85]

@mnapoli yes that's correct. I can handle that from there.

[mnapoli]

@Jean85 thank you!

[weierophinney]

@Jean85 Thanks - I dropped the ball on this, as I raised all of this right before the election cycle, and then forgot about it once elections were done. All we need is the discussion period + vote before we release, and when we do, we can do both releases in succession (1.1.0 and 2.0.0).

[crynobone]

PHP Fatal error: Declaration of Mockery_0_Illuminate_Container_Container_Illuminate_Contracts_Foundation_Application::has(string $id) must be compatible with Illuminate\Container\Container::has($id) in /home/travis/build/orchestral/imagine/vendor/mockery/mockery/library/Mockery/Loader/EvalLoader.php(34) : eval()'d code on line 30

This break existing code when implementation doesn't has scalar type-hint. Odd that the requirement just based on php >=7.2 when implementation classes may have different release cycle.

[roxblnfk]

PHP Fatal error: Declaration of Mockery_0_Illuminate_Container_Container_Illuminate_Contracts_Foundation_Application::has(string $id) must be compatible with Illuminate\Container\Container::has($id) in /home/travis/build/orchestral/imagine/vendor/mockery/mockery/library/Mockery/Loader/EvalLoader.php(34) : eval()'d code on line 30

This break existing code when implementation doesn't has scalar type-hint. Odd that the requirement just based on php >=7.2 when implementation classes may have different release cycle.

Looks like Mockery is doing something wrong. I think you need to create an issue in the Mockery page.

[Jean85]

This break existing code when implementation doesn't has scalar type-hint. Odd that the requirement just based on php >=7.2 when implementation classes may have different release cycle.

The requirement is more than fine. End users will not install this newer version if they are not running at least 7.2; so they will still rely on the previous version, which is still on spec and compatible.

The only way to break this is to composer update --ignore-platform-reqs, but that's a huge footgun, and you're on your own if you do that.

Looks like Mockery is doing something wrong. I think you need to create an issue in the Mockery page.

Nope, it's PHP < 7.2 that doesn't allow dropping arguments type and breaks the mock.

[GrahamCampbell]

Looks like Mockery is doing something wrong. I think you need to create an issue in the Mockery page.

Not a bug in Mockery. This only happens when you try to run the code on PHP older than 7.2.0.

[crynobone]

Not a bug in Mockery. This only happens when you try to run the code on PHP older than 7.2.0.

Unfortunately it's happening on php build 7.2, 7.3 and 7.4

https://travis-ci.org/github/orchestral/imagine/builds/751644184

[GrahamCampbell]

Oh, this is because the illiminate interface does not have the typehint, and mockery is generating code to satisfy the new interface.

https://3v4l.org/n8U4f

[crynobone]

Oh, this is because the illuminate interface does not have the typehint, and mockery is generating code to satisfy the new interface.

Yeah, by https://3v4l.org/n8U4f logic when 1.1 released this going to cause error on some projects and "php": "^7.2" constraint wouldn't help much. Every implementation that already has existing interface such as Illuminate would have would have attempt to comply on mid release cycle, I believe this is best to handle on 2.0 release if possible

[GrahamCampbell]

I think a 2.0 release would make the most sense too, actually, even if it is not considered "breaking" in all senses. Making it 2.0 makes everyone have to opt-in to upgrading.

[mnapoli]

A 2.0 release would disrupt the initial plan about the PSR upgrade though?

[Crell]

The general upgrade strategy specifically allows for 1.1/2.0 or a 2.0/3.0 transition, depending on the needs of particular specs.

[Jean85]

Isn't mocking the Illuminate interface the right choice there?