On Mon, Mar 2, 2020 at 9:00 AM Matthew Weier O'Phinney < mweierophin...@gmail.com> wrote:
> > > On Sun, Feb 23, 2020 at 1:58 PM 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.) >> >> 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 disagree with this analysis. > > If the specification indicates that an argument should be a string, then > passing anything other than a string for that argument is an error on the > part of the user. If the particular implementation allows it, then that > implementation is coded incorrectly. > > While I understand and appreciate the issue that Symfony has experienced, > I'd argue that their change to use strict_types fixed an issue with their > code, and Drupal then discovered they were incorrectly consuming the > Symfony API. If Symfony type-hinted as "string", but allowed non-strings > previously, this change is a bugfix. Yes, Drupal is left needing to change > their code after a minor release, but they should have been using it > correctly previously, and either casting to string ((string) $value) or > calling the object's `__toString()` method. > > Yes, PHP has allowed munging types when undefined or in weak mode. > Generally speaking, if the type is important, the implementation should: > > - when untyped, check the type explicitly > - when weakly typed, assume that the consumer is casting > - when strictly typed, require the consumer to cast > > It's that second point where I can see the potential argument around > "well, PHP will cast objects that define __toString()"; that is in fact > exactly what happens when an object defining __toString is encountered when > passed to a parameter defining a string typehint when strict_types is not > enabled. That said, _doing so is counter to the type hint_, and even your > IDE will tell you there's an error (e.g., intelephense raises "Expected > type 'string' Found '{Object type}'"); as a consumer, you shouldn't rely on > PHP's value munging _as you cannot know that strict_types is NOT enabled_, > unless your tests pass without munging. And even then, that's an indication > that you're passing a value that might be considered invalid in the future, > and you shouldn't rely on it. > > The problem with that second option (weak types, string typehint) is more > difficult from the _implementation_ aspect. Because PHP will cast the value > when it passes it to the method, there's no way to know if an object was > actually passed instead; you'll get a string. But this is true when you > pass any other scalar type as well. As such, the onus is on the _consumer_ > to pass the right values. > > I'd argue that if/when we update a specification package to use type > hints, we require that strict_types is enabled, as it gets rid of the > squishiness of that second case, and ensures that we can properly test the > type of the value passed to the method. Since adding scalar types requires > PHP 7 releases regardless, this is reasonable. > > But in no way do I think that if a specification currently defines a > typehint as a string that we should assume this should have meant > `string|Stringable`. If the string type hint is there, _current_ users > should be casting objects to string when passing _now_ in order to be > consuming the API correctly. Widening the type is something that would > require an errata vote. > I think this is a fair point, especially for something already implemented like a class in Symfony. I'd argue that given our wide reaching impact and the fact that we didn't necessarily discuss these things or explicitly disallow Stringables, we should be a little more pragmatic and apply a little more tact than simple disallowing Stringable everywhere and saying folks should've been aware. > > >> >> 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. >> >> I agree with a combination of B and C, it makes sense to apply string typehints where we know we absolutely don't want stringables and correct what's left potentially with errata when v8 gives us more tools. >> >> 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 tend to agree with option B as well. With most of the specs, there's > been uncertainty and confusion about what either the PHPDoc syntax or PHP > syntax would end up being, but the specs are very clear on the behavior. > Now that the PHP syntax is getting sorted, since `: self` will work for PHP > 7.4+, we can go with that, and then move to `: static` at a later date. > I agree with this, the B then C combination makes sense to me for the reasons stated. > > >> >> >> 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. >> >> I open the floor for comments, especially from the Core Committee folks. >> >> -- >> Larry Garfield >> la...@garfieldtech.com >> >> -- >> 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/c20ce8d8-d633-4629-b8b8-54742a11adcb%40www.fastmail.com >> . >> > > > -- > Matthew Weier O'Phinney > mweierophin...@gmail.com > https://mwop.net/ > he/him > > -- > 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/CAJp_myUrb_CsCrnzOT-YiFE-aBNFUxx5TDD-rhtJ-3pjFviJoA%40mail.gmail.com > <https://groups.google.com/d/msgid/php-fig/CAJp_myUrb_CsCrnzOT-YiFE-aBNFUxx5TDD-rhtJ-3pjFviJoA%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > -- 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/CANeXGWW7FQK-LyYCYveJi_cHcib%3DG8FSkLxc7-oqNBQgEOGa5w%40mail.gmail.com.
