Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New PHP 8.4 Syntax: Property Hooks #88

Open
Tracked by #97
KorvinSzanto opened this issue Jul 12, 2024 · 5 comments
Open
Tracked by #97

New PHP 8.4 Syntax: Property Hooks #88

KorvinSzanto opened this issue Jul 12, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@KorvinSzanto
Copy link
Contributor

KorvinSzanto commented Jul 12, 2024

Property hooks are landing in 8.4, the next version of PER-CS should cover them:

class Foo
{
    public function __construct(private User $user) {}

    public string $userName {
        get {
            return $this->user->userName;
        }
    }
}
@Crell
Copy link
Collaborator

Crell commented Jul 13, 2024

Unsurprisingly, my recommendation is for the style used in the RFC. Including allowing get to be all inline if it's an abbreviated get.

public string $userName { get => $this->user->userName; }

@samdark samdark added the enhancement New feature or request label Jul 14, 2024
@Crell Crell mentioned this issue Sep 3, 2024
9 tasks
@TimWolla
Copy link
Contributor

Unsurprisingly, my recommendation is for the style used in the RFC.

That brace placement would be inconsistent with methods, which have a newline before the opening brace.

@Crell
Copy link
Collaborator

Crell commented Oct 15, 2024

That's true. However:

  1. The point of hooks is to make coding easier and more compact for common cases. That means not forcing extraneous newlines where we don't have to.
  2. The RFC used inline and no one objected at that time.
  3. Control structures all use same-line braces. The spec is already inconsistent.
  4. PSR-2's decision to require a newline for classes and methods/functions was incredibly stupid, misguided, based on a bad representation of the data, and made without any regard for language consistency. It's the dumbest part of these coding standards, and the only reason we haven't changed it is because the breakage would be massive. Please, let's not continue that stupidity. 😄

@jrfnl
Copy link
Contributor

jrfnl commented Oct 16, 2024

The RFC used inline and no one objected at that time.

Sorry, but that really is a non-argument. People read and evaluate RFCs for the functionality which is being proposed. The code style used to present that functionality is absolutely irrelevant to an RFC and discussing that on the internals mailing list would be waved away within seconds.

PSR-2's decision to require a newline for classes and methods/functions was incredibly stupid, misguided, based on a bad representation of the data, and made without any regard for language consistency. It's the dumbest part of these coding standards, and the only reason we haven't changed it is because the breakage would be massive. Please, let's not continue that stupidity.

The language used here is pretty strong and IMO comes across as disrespectful to the people involved in that decision.

Whether you agree with decisions taken in the past or not, is irrelevant.

Code style is all about consistency and making thing easier to read. Advocating for deliberate inconsistency just because you don't agree with a past decision is not a good argument to back up a proposal.

If you don't agree with a past decision, challenge that decision and propose to change it, but don't make a code guideline inconsistent.

@edorian
Copy link

edorian commented Oct 16, 2024

Just my 2 cents and my heuristics on the matter having read the RFC, played with the 8.4RCs and prepared a couple of slides with code examples for talks.

Instinctively, both examples discussed here feel formatted correctly, the longform example only feels correct because I prefer braces on the same line for functions. Which clashes with how all other code looks.

Longform

With that in mind, I'd say for this standard to be consistent, the formatting could look like this:

    public string $userName
    {
        get
        {
            return $this->user->userName;
        }
        set (string $name)
        {
            $this->user->userName = $name;
        }
    }

Which I find very lengthy personally.

    public string $userName
    {
        get {
            return $this->user->userName;
        }
        set (string $name) {
            $this->user->userName = $name;
        }
    }

Feels more natural like a “function” (string $username properties) and an inline statement “get” inside that “function”.

Short closures / Arrow Syntax

public string $userName { get => $this->user->userName; }

Also feels ok and fitting to me.

  • match expressions are written in one line and this looks similar to that
  • We already have arrays with only a single element being written inline as well

Short closure with more than one accessor.

My main question would be on using both a set and a get with short syntax. The new line after $userName again feels fitting with the rest of the standard, despite my person preference.

public string $userName 
{ 
    get => $this->user->userName;
    set (string $name) => $this->user->userName = $name;
}

Summary

For me, the above feels consistent with how we treat single element arrays vs. multi-element arrays.

Reopening the discussion for this standard on “new line before { in general” doesn't feel productive and is something people can overrule on a per-project basis if need be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants