On Fri, Feb 23, 2024, at 4:50 PM, Stephen Reay wrote:
>> On 23 Feb 2024, at 22:58, Larry Garfield <la...@garfieldtech.com> wrote:
>>
>> On Fri, Feb 23, 2024, at 8:33 AM, Stephen Reay wrote:
>>>> On 23 Feb 2024, at 06:56, Larry Garfield <la...@garfieldtech.com> wrote:
>>
>>> Hi Larry,
>>>
>>> It's good to see this idea still progressing.
>>>
>>>
>>> I have to agree with the other comment(s) that the implicit
>>> `$field`/`$value` variables seem odd to me. I understand the desire for
>>> brevity and the potential future scope of reused hooks, but this
>>> concept seems to fly in the face of many years of PHP reducing "magic"
>>> like this.
>>
>> The "magic" that PHP has been removing is mostly weird and illogical type
>> casting. As noted, neither of these variables are any more "magic" than
>> $this.
>>
>> However, since it seems no one likes $field, we have removed it from the
>> RFC. Of note, to respond to your comment further down,
>> $this->{__PROPERTY__} will not work. The virtual-property detection looks
>> for the AST representation of $this->propName, and only that. Dynamic
>> versions that turn into that at runtime cannot work, as it needs to be known
>> at compile time.
>>
>
> I guess it's mostly irrelevant on single-use hooks anyway, but that
> sounds like a potential gotcha with reusable hooks, and I think it's
> worth making it very clear *now* in the RFC/docs that dynamic access
> like this won't work as expected (and why). Perhaps some other
> indicator on reusablele hooks can be used at that point, to signify if
> it's virtual or not.
Look, we can have a common variable that can be used in all hooks, or we can
minimize the "magic" names. We can't do both. Make up your mind. :-P
It's already been included in the RFC that only explicit property name usage
will work.
As noted, reusable hook "packages" a la Swift is a potential future scope. At
that point having some common variable name is probably sensible, but we can
color that bikeshed when we get to it.
>> For $value, however, we feel strongly that having the default there is a
>> necessary part of the ergonomic picture. In particular, given your comments
>> here:
>> public function __construct(
>> public string $phone { set(string $phone) => $this->phone =
>> $this->sanitizePhone($phone); }
>> public string $phone { set => $this->sanitizePhone($value); }
>> ) {}
>>
>> Again, it's absolutely no contest for me. I would detest writing the longer
>> version every time.
>>
>
> I think you're making slightly misleading comparisons there, by picking
> the two extremes (and ignoring the possibly for shorter explicit names)
> - the explicit parameter name surely doesn't preclude the use of
> return-to-set functionality? So the comparison could equally be:
>
> ```
> public function __construct(
> public string $phone { set => $this->sanitizePhone($value); }
> public string $phone { set(string $v) => $this->sanitizePhone($v); }
> ){}
>
> ```
Yet nearly all coding standards and recommendations (outside of Go) say to not
use single-character variable names, so I wouldn't expect that to be common.
Even if it's not as big of a difference, it's still two places to specify the
type and two places to specify the variable name. That is, two places to get
either one wrong. Constructor Promotion is one of the best features of PHP 8,
precisely because it eliminates that kind of duplication. I see no reason to
mandate redundancy and duplication in code.
>> If PHP has been moving away from weird and inexplicable magic, it's also
>> been moving away from needless boilerplate. (Constructor promotion being
>> the best example, but not the only; types themselves are a factor here, as
>> are arrow functions.) As the whole point of this RFC is to make writing
>> common code easier, requiring redundant boilerplate for it to work is
>> actively counter-productive.
>>
>> So what I'd suggest instead is "specify the full signature if you want a
>> custom name OR wider type; or omit the param list entirely to get a
>> same-type $value variable, which 99% of the time is all you need." We get
>> that in return for documenting "$value is the default", which for someone
>> who has already figured out $this, should be a very low effort to learn.
>
> I get your point, and to expand on what I said in the first email - if
> removing the implicit mode would mean people vote against the RFC, then
> that's a worse result, IMO (this is why I suggest a secondary vote -
> perfect is th enemy of good and all that). I personally think the
> implicit variables will result in less-readable code, but I also know
> that I'm free to not use the implicit parameter in code that I write,
> so I trust those with a more accurate finger on the pulse of the voters
> to know whether the implicit `$value` parameter will help or hinder the
> RFC to pass.
As has been noted numerous times, there is basically zero way to tell what
people will vote before they do unless they explicitly say so on list, and the
list has had maybe a half dozen voters comment at most. So, I truly have no
clue if $value is the pass/fail trigger for someone, or which direction. This
is a long-standing issue with the PHP dev process that Internals has shown no
desire to address.
The problem with secondary votes is that they come off as "meh, I couldn't be
arsed to decide here." Based on comments on the list, some non-small number of
voters see secondary votes as a good "crowdsourcing" answer, and another
non-small number of voters see them as abdicating responsibility and want
opinionated RFCs. I have no idea which to go for. :-/
But in this case, the opinionated RFC position says "making people type the
same things multiple times on every usage when there's no reason to is silly."
>>> Also, a small nitpick: The link to your attributeutils repo in the
>>> examples page, is broken, and it would be nice to see a few examples
>>> showing the explicit version of the hooks.
>>
>> Link fixed, thanks. What do you mean explicit version of the hooks?
>
> I meant hooks that don't use the implicit "magic" variables (i.e.
> specify the parameter on set() specify the full property name when
> accessing the backing store). Given your first response I guess this
> request is a little moot, assuming the examples are updated to remove
> use of `$field`. Maybe the RFC or an example already covers it and I
> skipped over it, but it'd be good to make it clear *exactly* what the
> equivalent of an implicit `$value` parameter is.
Once others weigh in on either of the proposals I've made in this thread, I can
update the RFC accordingly.
--Larry Garfield
