On Mon, 16 Jan 2023 at 15:36, Christian Schneider <cschn...@cschneid.com> wrote:
> Am 16.01.2023 um 14:39 schrieb G. P. B. <george.bany...@gmail.com>: > > On Sun, 15 Jan 2023 at 20:58, Christian Schneider <cschn...@cschneid.com> > wrote: > > Some comments: > > - I am not convinced that we should introduce a third way of providing > parameters to setcookie(). I don't think this function is used often enough > in common code to add yet another iteration of the API. Assuming there is 1 > to 2 places in your framework using this I don't think many bugs will go > unnoticed. Adding a warning to illegal 'samesite' values in $options would > IMHO be enough if stricter checking is wished for. > > > > How is this providing a third way of providing parameters to > setcookie()? As it is, if I want to use named arguments and the typed > parameters, I cannot set the SameSite attribute. > > I've heard multiple people waste time due to a SameSite bug because they > made a typo, and it took them way too long to figure out the issue as they > thought they had a server configuration problem. > > Adding a warning for invalid values is effectively turning that option > into an Enum, which at this point one might as well have a proper Enum. > > Maybe third API was misleading, what I mean is that we had the (old) way > of positional arguments for options which did not include something like > samesite and the (new) way with the options array. So I would assume most > code was migrated to $options. > Now you are suggesting of updating the old API with an additional > positional argument which would mean changing the array-form back to > positional. > Is your plan to deprecate the $options-API at some point in the future? > Having two APIs offering the same functionality seems a bit confusing to me. > I personally don't plan on deprecating the array $options API. If two APIs that provide the same functionality are too confusing, then the current incomplete way of passing parameters should be deprecated at some point. However, until then, having symmetry between both APIs is better for people who prefer to have parameters and use named arguments than the $options array. > Alternatively the 'samesite' option in $options could accept > SameSite::Lax. This would accomplish (almost) the same with a smaller > change. Disclosure: I like arrays for options as they allow for array > addition etc. Something similar can be done with ...$options, true :-) > That is actually a good idea to add support for the array version to accept the new enum, I'll amend the implementation. > Side-Note: Isn't SameSite a very generic name in the global namespace? I'm > not sure what the PHP policy is here. > AFAIK the global namespace is "owned" by PHP so that shouldn't be an issue per the usual policy. > > - I don't like the camelCase of $sameSite as this is different from > all the other parameters, e.g. $expires_or_options (yes, this is a > pseudo-parameter name, I know) and $httponly. Looking at a couple of > functions in the standard PHP set I didn't see any $camelCase. > > > > ACK, it should probably be in snake case and be $same_site > > Looking at $httponly I would have expected $samesite. > I suppose it can be either or, but trying to improve the name of a not yet existing parameter seems better than following the weird naming scheme. But I'm not hard set on the name here. > > - A more generic question: How are Enums handled concerning future > additions of values vs. BC compatibility? What is the migration plan there > if one wants to support both old and new PHP versions? > > > > The parameter is optional, so I don't understand what migration plan you > need? > > If in the, unlikely, event that a new value is added this should be > added as a case in the enum in the next minor version of PHP. > > In the, again unlikely, event that a value is deprecated, the > corresponding enum case should also be deprecated following the normal PHP > deprecation process. > > > > I only decided to make this an enum because I deem it very unlikely for > a new valid attribute value to be introduced, the new IETF RFC clarifies > and amends the behaviour of the 3 valid attribute values that have always > been the same since 2016. > > I was talking about extending Enums, not the parameter and not SameSite > specifically. > Are there any Enums in core PHP APIs where new values could be added in > the future and how is the plan for code supporting multiple PHP version > there? This was just something which crossed my mind when thinking about > APIs with Enums. This is not really related to this RFC so I understand if > you want to ignore this part :-) > I might be again misunderstanding, but one cannot extend an enum as they are final classes under the hood. Currently, the only other native enum is the one that was added with the Randomizer Additions RFC [1] so this topic hasn't come up yet as the enum for ext-random is definetely complete. Best, George P. Banyard
