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.

Sign up for GitHub

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

Jump to bottom

[11.x] Handle aliases when appending attribute accessor #54589

Closed
wants to merge 3 commits into from
Closed

[11.x] Handle aliases when appending attribute accessor #54589

wants to merge 3 commits into from

Conversation

jackbayliss
Copy link
Contributor

@jackbayliss jackbayliss commented Feb 12, 2025 •
edited
Loading

Closes #54570

TLDR:

  • Using the getFooBarQuzAttribute way, appending with any alias works when casting to array
  • Using the class way return Attribute::make it causes an exception for some aliases that work with the above way
  • This makes it so either way you do it, its the same behaviour

Why

Currently if you use the new style attribute accessor in a method, such as:

protected function FooBarBaz(): Attribute
{
    return Attribute::make(
        get: fn () => 'yay',
    );
}

As we know, we can access it via the model with various aliases, such as :

> App\Models\Post::find(1)->foo_bar_baz
= "yay"
> App\Models\Post::find(1)->foobar_baz
= "yay"
> App\Models\Post::find(1)->foo_barbaz
= "yay"
> App\Models\Post::find(1)->foobarbaz
= "yay"
> App\Models\Post::find(1)->_foo_barbaz
= "yay"

Appending works if you use the full snake of the method:

> App\Models\Post::find(1)->first()->append('foo_bar_baz')->toArray()
= [
   "id" => 1,
   "title" => "AVTPC4BJnl",
   "created_at" => "2025-02-10T23:27:01.000000Z",
   "updated_at" => "2025-02-10T23:27:01.000000Z",
   "foo_bar_baz" => "yay",
 ]

However some aliases do not work, when using the this way of defining a attribute, which do work when you use the alternative way ie getFooBarBazAttribute():

> App\Models\Post::find(1)->first()->append('foobarbaz')->toArray()

   BadMethodCallException  Call to undefined method App\Models\Post::getFoobarbazAttribute().
   
> App\Models\Post::find(1)->first()->append('_foo_barbaz')->toArray()

   BadMethodCallException  Call to undefined method App\Models\Post::getFooBarbazAttribute().

Where as if you used the alternative get notation:

protected function getFooBarQuzAttribute(): string
{
   return 'boo';
}

It works with all aliases:

> App\Models\Post::find(1)->first()->append('foobarquz')->toArray()
= [
    "id" => 1,
    "title" => "AVTPC4BJnl",
    "created_at" => "2025-02-10T23:27:01.000000Z",
    "updated_at" => "2025-02-10T23:27:01.000000Z",
    "foobarquz" => "boo",
  ]
> App\Models\Post::find(1)->first()->append('_foo_barquz')->toArray()
= [
    "id" => 1,
    "title" => "AVTPC4BJnl",
    "created_at" => "2025-02-10T23:27:01.000000Z",
    "updated_at" => "2025-02-10T23:27:01.000000Z",
    "_foo_barquz" => "boo",
  ]

What it fixes

This PR fixes it so all the aliases work, regardless of how you're using the accessor - ie getXAttribute or the class, which is what I would expect. Currently it does not work if you use the class way of doing it.

with fixes examples below:

> App\Models\Post::take(3)->get()->append('foobarbaz')->toArray()
= [
    [
      "id" => 1,
      "title" => "AVTPC4BJnl",
      "created_at" => "2025-02-10T23:27:01.000000Z",
      "updated_at" => "2025-02-10T23:27:01.000000Z",
      "foobarbaz" => "yay",
    ],
    [
      "id" => 2,
      "title" => "UmcAQ1HTn8",
      "created_at" => "2025-02-10T23:27:06.000000Z",
      "updated_at" => "2025-02-10T23:27:06.000000Z",
      "foobarbaz" => "yay",
    ],
    [
      "id" => 3,
      "title" => "CbYubuFL2e",
      "created_at" => "2025-02-10T23:27:15.000000Z",
      "updated_at" => "2025-02-10T23:27:15.000000Z",
      "foobarbaz" => "yay",
    ],
  ]
> App\Models\Post::find(1)->first()->append('_foo_barbaz')->toArray()
= [
    "id" => 1,
    "title" => "AVTPC4BJnl",
    "created_at" => "2025-02-10T23:27:01.000000Z",
    "updated_at" => "2025-02-10T23:27:01.000000Z",
    "_foo_barbaz" => "yay",
  ]
  
> App\Models\Post::find(1)->first()->append('foobarbaz')->toArray()
= [
    "id" => 1,
    "title" => "AVTPC4BJnl",
    "created_at" => "2025-02-10T23:27:01.000000Z",
    "updated_at" => "2025-02-10T23:27:01.000000Z",
    "foobarbaz" => "yay",
  ]

In a real world situation, it just means you can append the attribute with whatever alias you prefer, keeping it consistent with how you're accessing it from the model, which is a win win? And if you're switching from the get notation to the class way, it will continue to work.

Doesn't break anything from my understanding, and just resolves this particular issue with aliasing, which already works via the other way ie getFooBarBazAttribute() 🕺🏻

Hopefully tests are OK, but any issues let me know 🫡

@github-actions GitHub Actions
Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@jackbayliss jackbayliss marked this pull request as ready for review February 12, 2025 23:27
@donnysim
Copy link
Contributor

It's everyone's choice, but consistency is key in maintenance, having random ways of accessing a value is not that great in practice.

@jackbayliss
Copy link
Contributor Author

jackbayliss commented Feb 13, 2025 •
edited
Loading

@donnysim

You can use any alias already if you use the getFooBarBazAttribute() way of defining the attribute. This just keeps it in line with that when using the class way🫡

@Propaganistas
Copy link
Contributor

FYI: I opened the reported bug (#54570) because I was converting getXAttribute() methods to the Attribute syntax. I've been accessing them consistently using one specific snake variant all over the place, but suddenly tests started to fail.

Thanks @jackbayliss for taking a crack on this.

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

@jackbayliss jackbayliss deleted the fix_append_alliases branch February 13, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent accessor attribute name conversion
4 participants
@jackbayliss @donnysim @Propaganistas @taylorotwell