On Sun, 23 Feb 2020 at 20:58, Larry Garfield <la...@garfieldtech.com> wrote:
> OK, this has been sitting far too long now. Mostly my fault, but no one > else seems to be pressing the issue, either... > > Background: Back in December I filed a pair of PRs to update PSR-13 to use > type hints, per the recently-passed evolution bylaw. I figured it would > make a good test case to flush out edge cases, and I was right. > > PRs: > https://github.com/php-fig/link/pull/6 > https://github.com/php-fig/link/pull/7 > > From the discussion here, there, and elsewhere (musical references FTW), > I've identified two primary concerns that should be *globally* resolved for > any spec updating in this fashion, not just PSR-13, although PSR-13 is the > case in point. > > 1) string parameters are a potential BC break in strict mode; does that > require a v2 rather than v1.1? > > This is an issue that Symfony ran into when upgrading some of its own > libraries, and prompted the "Stringable" RFC from Nicolas for PHP 8. (cf > https://wiki.php.net/rfc/stringable, currently in voting as I write this, > and seems likely to pass but not overwhelmingly.) > There is a 75% majority to add this and the vote closes in 3 days. It may not be unanimous but it clears the threshold pretty safely. > The specific issue is that if a method takes an untyped string parameter, > right now that implicitly means `string` or "object with __toString"... at > least most of the time. > > If it is typed `string`, then in weak mode it means the same, although the > object will get cast to a real string at call time rather than sometime > later; in strict mode, a Stringable object is no longer allowed. That > means a nominal API change, which should necessitate a new major of the > interface package (v2, not v1.1). (According to Nicolas on the PHP > Internals News podcast (https://phpinternals.news/39), Symfony missed > this implication originally and inadvertently broke Drupal, which was > passing a stringable object to a method to which they'd added a string type > hint. That was the impetus for the Stringable RFC.) > > Example (try flipping the declare statement on and off): > > https://3v4l.org/0dpsJ > > It's been pointed out that since by design an implementation can continue > to omit the parameter type, it's a BC break by the implementer, not the > spec, and thus the spec has only a non-breaking change and can be a 1.1. I > disagree; for one, I don't expect most implementers to be thinking of that > sort of nuance. Honestly I likely wouldn't until it tripped someone up. > For another, we *want* implementers to be, in the long run, fully > implementing the spec and not dropping random parameters, even if the > language allows it. That would result in some implementations accepting > stringable objects and others not, which is directly contrary to the goal > of interoperability. > I'll reiterate but adding argument type hints to the interfaces is not a BC break, even with strings. Because it is indeed an implementation concern as I've said on the PR before. Two things, it seems you are dismissing implementers to not consider this, and if they don't someone will rapidly point it out to them; as in the Symfony case. Implementations do not need to "drop" argument type hints, as they don't have them in the first place. Secondly with PHP 8 (I'm totally assuming here that the Stringable interface will pass the vote given it's clearance) they can widen the argument type to string|Stringable from the string argument type of the interface. The only thing what that means is that an implementation may just need to delay adding type hints for string arguments in their implementation while they can still add all the other ones. Now if the concern is *that* big that implementer will not think about this (which I find to be slightly insulting) there is always the upgrade path of: - Version 1.1.0, to add argument types for all types except string with a PHP 7.2 requirement - Version 1.2.0, to add the string|Stringable argument type to the remaining ones with a PHP 8.0 requirement. > I believe the same issue exists for string return types as well, although > it's not quite as bad as implementers can cast their return values to > strings explicitly before returning them rather than putting the onus on > callers. > > I see a few ways we could go here: > > A) Add `string` hints where appropriate, release it as v1.1 (requires > 7.2), and say caveat emptor. > > B) Add `string` hints where appropriate, release it as v2.0 (requires > 7.2), and say "that's what a Major release means." > > C) Wait and see if the Stringable RFC passes; if it does, then we could > hold for PHP 8 and then allow whoever is upgrading a spec to add either > `string` or `string|Stringable` as they feel is appropriate to the spec > (requires 7.2 or 8.0 as appropriate). In this case it would definitely be > a v2, since it would be a conscious and deliberate narrowing of allowed > values to use `string` instead of `string|Stringable`. > > Given that some cases likely should be just `string`, I would favor some > combination of B and C, allowing-case-by-case decisions on whether > `Stringable` is appropriate to allow. Off hand I'm not sure which I'd > prefer for PSR-13; I've not thought that through yet. The only advantage > of A is that it does get the 1.1 release into people's projects really > fast, in a compatible way. > *How much that's an issue (given that requires: 1.x|2.x is legal in > Composer) I am not sure.* That's assuming implementations, which may not be maintained any more, to update their requirements. > 2) `return self` is a breaking problem until 7.4, but gets better in 8.0. > > PSR-13, like a few other specs, has methods that return $this. They are > currently untyped, just documented. I initially attempted to add `return > self` to those methods in the v2/3 PR (#7), but that in fact breaks in PHP > < 7.4, because of the weaker covariance. Example: > > https://3v4l.org/ttop8 > > Possible ways we could handle this: > > A) Do not add `return self` types until a 7.4-compatible version. That > would mean a 3-step upgrade; v2 for parameter types (requires 7.2), v3 for > other return types (requires 7.2), v4 for return self cases (requires 7.4). > > B) Skip the "return types other than self" step. So in PSR-13's case that > would mean v2 for parameter types (requires PHP 7.2) and a v3 that adds all > return types (requires 7.4). > > C) Wait for PHP 8 to release the return-type step, which is slated to have > support for `return static`, which is what's really intended here anyway > and is more accurate than `return self` entirely. This could be done with > (CA) or without (CB) an intermediary step to add the non-self return values. > > I think my preference is for CB: Do parameter types now, then wait until > December to do a v3 with `return static` and a PHP 8.0 dependency. As a > matter of precedent, that would imply doing the same for PSR-7 and any > other spec that has a documented `@return $this` or similar. > I'll just say it on-list once again but, return types are way less of an issue since PHP 7.4 as implementations can add them themselves, and this requires a major version anyway for both the interface and the implementation. (And with the times everything things to take with FIG waiting until the release of PHP 8 for a *unique* major bump with the interfaces instead splitting it into two makes more sense, as implementations need to release a new version anyway to add return types) > I note that both of these issues have a "do nothing, PHP 8 will save us" > option. I do not think "wait forever" is a good general strategy on > upgrading specs, but in these two particular cases given what we know at > this moment they seem like viable options. We should be careful of the > precedent, however, of how good an upgrade has to be before it's good > enough for us to pin to a given language version, given the subtle changes > that may sometimes result. > This is plain false, PHP 8 will not safe us for argument type hinting because until the interfaces add them it is *impossible* for implementations to add them. > I open the floor for comments, especially from the Core Committee folks. > > -- > Larry Garfield > la...@garfieldtech.com > I'll add another related comment, from what I personally remember, the reason why it took *several years* for even the proposal to be able to update the PSR interfaces was to prevent "dependency hell" (i.e. impossible to solve version constraints on the PSR interface with different packages) as the PSR interfaces are requires in a lot of packages. This proposal of needing a V2 for argument types and a V3 (or even a V4) for return types makes this sight of dependency hell *way* more likely then just respecting semver versioning and the implementers to know what they are doing. Finally, it is way less "urgent" to add return types to the interfaces as with PHP 7.4's covariant return type feature implementations can add them without the interfaces having them. Personally, I don't think we even need it if we follow through with the premise of not even letting the possibility of dependency hell to arise. As a V2 for adding them may still cause it. Best regards George P. Banyard -- You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group. To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CAFPFaMKeRtB4NDT-PRRO1RLkGX3y%2BX219W_QrnWw7RNpwf_kOg%40mail.gmail.com.
