Skip to content

Conversation

Crell
Copy link
Collaborator

@Crell Crell commented Dec 2, 2019

Whether this gets tagged as 2.0 or 1.1 is still open, I think, but here's the code.

@Girgias
Copy link

Girgias commented Dec 3, 2019

This should be tagged as 1.1, as it is fully backward compatible with 1.0 and provided the major benefit of allowing implementations to finally type argument types.

@alcaeus
Copy link

alcaeus commented Dec 19, 2019

This should be tagged as 1.1, as it is fully backward compatible with 1.0 and provided the major benefit of allowing implementations to finally type argument types.

For packages only containing interfaces, this is true. When a package provides an implementation, this is no longer true, as you are introducing a subtle but far-reaching change to the contract. @param string $foo is not the same as string $foo - most notably when the calling code is operating under strict typing rules (which some coding standards actively enforce). This can cause fatal errors for code that worked perfectly fine before.

@Girgias
Copy link

Girgias commented Dec 19, 2019

This should be tagged as 1.1, as it is fully backward compatible with 1.0 and provided the major benefit of allowing implementations to finally type argument types.

For packages only containing interfaces, this is true. When a package provides an implementation, this is no longer true, as you are introducing a subtle but far-reaching change to the contract. @param string $foo is not the same as string $foo - most notably when the calling code is operating under strict typing rules (which some coding standards actively enforce). This can cause fatal errors for code that worked perfectly fine before.

Well I'm talking about this PSR which only defines interfaces, so I think we agree that this should be tagged as 1.1?

@alcaeus
Copy link

alcaeus commented Dec 19, 2019

That depends. Other FIG libraries may declare classes and they may want a consistent strategy 🤷‍♂️

@Girgias
Copy link

Girgias commented Dec 19, 2019

I personally prefer to use a minor upgrade as often as possible, and to my knowledge only PSR-3 has some implementations?

Moreover if there is going to be a V2 anyway to add return types it feels logical to add the parameter types to the PSR implementations at the same time in the V2.

@Crell
Copy link
Collaborator Author

Crell commented Dec 19, 2019

The two-step upgrade is deliberate and necessary for BC. See the original blog post: https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/

It's going to be a 2-step upgrade. It's just a question of whether it's 1.1 and 2.0, or 2.0 and 3.0.

@alcaeus
Copy link

alcaeus commented Dec 19, 2019

For the doctrine project, we’ve decided to add parameter types in a major release and return types in the next major: this is to make the update to typed releases explicit for downstream consumers. The blog post you wrote was the basis for our decision.

@Crell
Copy link
Collaborator Author

Crell commented Dec 24, 2019

I spoke with @alcaeus a few days ago. I think his point on string vs. string|\__toString() is very valid, and that pushes me toward making this a 2.0.0, not a 1.1..

@Girgias
Copy link

Girgias commented Dec 25, 2019

I spoke with @alcaeus a few days ago. I think his point on string vs. string|\__toString() is very valid, and that pushes me toward making this a 2.0.0, not a 1.1..

I don't see the issue? a __toString still generates a string so type hinting against string is not an issue. Worst case the implementation should not use strict types to enforce the type hint in that way as a weak type string will happily convert an object to string (if it can). Therefore 1.1 is still appropriate.

@alcaeus
Copy link

alcaeus commented Dec 25, 2019

The strict type declaration is not taken from the code declaring the class (in this case the FIG library), but from the calling code. Since lots of coding standards are advocating using strict type declarations and people do this without thinking of the implication, it’s better to be conservative and avoid the subtle BC break. After all, it’s no big difference, the only one being that users make a conscious decision to allow the next major version.

@Girgias
Copy link

Girgias commented Dec 25, 2019

The strict type declaration is not taken from the code declaring the class (in this case the FIG library), but from the calling code. Since lots of coding standards are advocating using strict type declarations and people do this without thinking of the implication, it’s better to be conservative and avoid the subtle BC break. After all, it’s no big difference, the only one being that users make a conscious decision to allow the next major version.

This is still on the implementation side, because as you said the interface declaring strict types has no effects.
Thus it's on the implementation side to bump the version number not on the interface declaration.
There is no subtle BC break at all by having this as a 1.1 version.

@Crell
Copy link
Collaborator Author

Crell commented Dec 27, 2019

// foo.php
class Foo implements LinkProviderInterface {
  public function getLinksByRel(string $rel):array { ... }
}
// script.php

$f = new Foo();

$f->getLInksByRel($stringableObject);

Without the type hint, script.php works regardless of whether it's in strict or weak mode.

When the type hint gets added, script.php will break IF it's in strict mode, but not in weak mode.

It's small and subtle, yes. In this particular case I cannot imagine why someone would be using a stringable object for the rel name. But they could be, and what we do here is going to set a standard for all other PSRs in the future. Think of PSR-7, or PSR-3, which are very widely used and a subtle change like that could break an awful lot of code... in particular, code that's using strict mode, which is what we want to encourage people to do.

As a policy, since there is a non-zero potential for breakage I believe we should just go v2 as a FIG practice. Remember, FIG interfaces are by nature very high impact, so we have to be extra special careful with them, moreso than most other projects would be.

@Girgias
Copy link

Girgias commented Dec 27, 2019

// foo.php
class Foo implements LinkProviderInterface {
  public function getLinksByRel(string $rel):array { ... }
}
// script.php

$f = new Foo();

$f->getLInksByRel($stringableObject);

Without the type hint, script.php works regardless of whether it's in strict or weak mode.

When the type hint gets added, script.php will break IF it's in strict mode, but not in weak mode.

It's small and subtle, yes. In this particular case I cannot imagine why someone would be using a stringable object for the rel name. But they could be, and what we do here is going to set a standard for all other PSRs in the future. Think of PSR-7, or PSR-3, which are very widely used and a subtle change like that could break an awful lot of code... in particular, code that's using strict mode, which is what we want to encourage people to do.

As a policy, since there is a non-zero potential for breakage I believe we should just go v2 as a FIG practice. Remember, FIG interfaces are by nature very high impact, so we have to be extra special careful with them, moreso than most other projects would be.

I don't see the point of this example? This example is invalid PHP and will always be because it breaks LSP. You cannot create an implementation of the LinkProviderInterface that currently type hints the parameter, the added return type is valid as of PHP 7.4.

Therefore it is the responsibility of the implementing libraries to upgrade to a V2, NOT FIG. As declaring argument type hints does not break ANY current implementations as they ALL need to have NO type hints for arguments. Thus there is no concern what so ever about weak or strict types.

Also I would like to point out creating an unnecessary V2 just to add argument type hints (because it clearly is) is more likely to cause dependency hell than anything else which from my understanding is why FIG has been so reluctant to release an update to these interfaces.

I don't mind clarifying further but I can assure you adding argument type hints as a version 1.1 is in no way a BC break. This is not the same for return types.

@jpickwell
Copy link

I would say this is a BC, and thus a major version bump, because of the change to the minimum PHP version alone.

@Girgias
Copy link

Girgias commented Jun 2, 2020

I would say this is a BC, and thus a major version bump, because of the change to the minimum PHP version alone.

Semver disagrees with you here:
https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

@Crell Crell merged commit 94d1709 into php-fig:master Feb 3, 2021
@Crell Crell deleted the v2 branch February 3, 2021 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants