moebrowne's avatar

MoeBrowne

moebrowne

Member since

209

Total Reputation

4

Total Arguments

17

Total Votes for Arguments

Arguments and votes

1

This RFC needs a re-work. While I support the idea, I don't think this is the right approach. The fact that the RFC is so long is a massive red flag. For example there are 4 different ways to define hooks:

class User
{
    public string $username {
        set(string $value) {
            $this->username = strtolower($value);
        }
    }
}
class User
{
    public string $username {
        set(string $value) {
            $field = strtolower($value);
        }
    }
}
class User
{
    public string $username {
        set => strtolower($value);
    }
}
class User
{
    public function __construct(
        public string $username { set => strtolower($value); }
    ) {}
}

Why?!

This is not what PHP needs.

Share:
Read the RFC: Property Hooks moebrowne avatar
moebrowne
voted no
2

It feels like it adds a huge amount of complexity just to avoid getter/setter methods. Also, I'm not sure it's a good thing to lose the explicit differentiation between a property access and method call.

Note: setter methods are something I almost never use in the first place, since it's typically better to have immutable value objects.

Share:
Read the RFC: Property Hooks theodorejb avatar
theodorejb
voted no
25

This kind of code looks very appealing when performing very simple (and anemic) CRUD operations on a model, but it has a very short trajectory when the code gets a little more complex.

Triggering events is shown as an example of an advantage of this feature, but it's clearly a bad idea: https://3v4l.org/ZCVpG#v8.2.11

Also, if you need a centralized validation around an attribute, a value object is a way better option: https://3v4l.org/QWm8c#v8.2.11

I think that there are a ton of better requests with a lot more of value to achieve this in userland code.

Also, this idea is very overlooked, and would require a lot of internals work after to cover all the edge cases. How should child classes behave? Are they overridable? How do you know if there is a setter behavior defined already? How should traits behave? Is there a way to call the parent class setter/getter? Would that be overriden or just called by default?

Also, in response to some comments:

It could be used by ORMs like Laravel Eloquent Model that has cast and other hooks to transform data on get/set.

Good ORMs should provide mechanisms for this!

However, I actually often find the need for more fine-grained control over input and output, but adding methods feels so heavy-weight.

This is the perfect case for value objects to kick in. You want behavior (input validation, output transformation maybe) at an atomic member, that could be exactly this but atomically standalone, and therefore reusable in other cases, and very easily testable.

Share:
Read the RFC: Property Hooks devnix avatar
devnix
voted no
17

While I'm not against the concept in general, this implementation is not well-done. The $value variables comes from nowhere and makes it confusing - it looks like an undefined variable and a quick glance at it, was the first thing I thought - where is $value coming from? Why not just use $name within that block? Same with $field? Why not just return the value that is going to be set? This is one of the more confusing RFCs I've seen and does not follow the PHP-code style, makes things confusing for both new and experienced coders. In it's current format, I can't approve with good conscience.

Share:
Read the RFC: Property Hooks jim avatar
jim
voted no
11

I think complaints about the syntax being messy are really about the array manipulation functions being inherently hard to format nicely.

To be clear I agree that the proposed example isn't great, nested closures are never going to win any readabillity prizes. If however we look at any non-array based manipulation I think the readabillity is objectively better:

$name = 'my_user_name'
    |> fn (string $string): string => str_replace('_', ' ', $string)
    |> strtolower(...)
    |> ucwords(...)
    |> trim(...);

Or without first class callables:

$name = 'my_user_name'
    |> fn (string $string): string => str_replace('_', ' ', $string)
    |> fn (string $string): string => strtolower($string)
    |> fn (string $string): string => ucwords($string)
    |> fn (string $string): string => trim($string);

Bonus: this also adds runtime type checks to each step. Eg strreplace returns string|array

I can't imagine anyone would think this is better:

$name = trim(
    ucwords(
        strtolower(
            str_replace('_', ' ', 'my_user_name')
        )
    )
);
Share:
Read the RFC: The Pipe Operator moebrowne avatar
moebrowne
voted yes
83

With First-class callable syntax available since 8.1, it would now be possible to write it as below, which is much better then string names of functions:

$result = "Hello World"
    |> htmlentities(...)
    |> str_split(...)
    |> fn($x) => array_map(strtoupper(...), $x)
    |> fn($x) => array_filter($x, fn($v) => $v != 'O');
Share:
Read the RFC: The Pipe Operator pronskiy avatar
pronskiy
voted yes
33

I see no immediate benefit of the proposed solution over the userland implementations. The RFC mentions a shopping cart example, but I don't think that's cleaner than using league/pipeline or Laravel's pipeline.

It's a bit messy for the simpler examples as well.

Share:
Read the RFC: The Pipe Operator ju5t avatar
ju5t
voted no
64

For me, the most important argument is that the pipeline pattern is a tried and tested pattern, that this RFC builds upon. A couple of examples:

This RFC adds syntax to make using these kinds of pattern much more convenient.

On top of that, there's the argument that multiple modern languages support a pipe operator:

Finally, I've had numerous occasions where a pipe operator would simplify my own code — I have more than a handful real life cases where this would be useful.

Share:
Read the RFC: The Pipe Operator Contributor brent avatar
brent
voted yes
1

I've written DefaultImplementation traits to implement common interface functionality one too many times. Would love to see this as a feature.

Share:
Read the RFC: Interface Default Methods moebrowne avatar
moebrowne
voted yes
1

I think the potential confusion this adds is not worth the small amount of extra code that would need to be written to use an anonymous function

Share:
Read the RFC: Short Closures 2.0 moebrowne avatar
moebrowne
voted no
8

mmh, seems to me that just replacing function by fn and having such a different behaviour is risky

Share:
Read the RFC: Short Closures 2.0 remivasco avatar
remivasco
voted no
5

Auto capture by-value is clear enough with a single expression closure, but I think for multiple statement functions it could be confusing to have to keep track of scope. Being explicit (with use()) makes it simple and concise.

If this were to pass, I think the syntax should continue to include the arrow to signify the auto capture by-value intention. Just like in JavaScript, the arrow is used whether single expression or multiple, and the use of brackets is what separates them.

Share:
Read the RFC: Short Closures 2.0 corey avatar
corey
voted no
24

The 'use' statement clarifies the scope for me. So a proposal like this could have the side effect of mixing scope which would lead to a confusing code.

Share:
Read the RFC: Short Closures 2.0 nabeel avatar
nabeel
voted no
10

Technically writing a trait isn't a showstopper, but it adds cognitive load because the developer now must know/remember using this Interface requires adding this Trait. PHP should focus more on development experience!

Share:
Read the RFC: Interface Default Methods eugen avatar
eugen
voted yes
8

It will solve having to create traits to add a default implementation when creating interfaces and keeping it nicely together improving the DX

Share:
Read the RFC: Interface Default Methods Carakas avatar
Carakas
voted yes
39

Creating traits for default implementation is just a pain. I want syntactic sugar

Share:
Read the RFC: Interface Default Methods marc avatar
marc
voted yes
55

I wrote down some thoughts on this RFC on my blog. I think it's worth rethinking our current definition of what "an interface" is. Especially since many languages are interface default methods as their way of multi-inheritance.

Share:
Read the RFC: Interface Default Methods Contributor brent avatar
brent
voted yes
RSS Feed Contribute Watch on YouTube Our License
© 2024 RFC Vote. This project is open source. Contribute and collaborate with us!