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

Add hasAttribute method to Eloquent Models #44717

Conversation

geisi
Copy link
Contributor

@geisi geisi commented Oct 24, 2022

Hello Laravel Team,

With Laravel's new strict Model::preventAccessingMissingAttributes Eloquent configuration, there are some problems with 3rd party packages when these packages want to access optional attributes.

To prevent MissingAttributeException, it would be handy to have a central way to check if an Eloquent Model attribute exists or not.

This PR introduces a new hasAttribute(string $key): bool method on Eloquent models to check if a model has an attribute configured. This also could be archived in the 3rd party package code. But I think it would be helpful if this gets added to the core. So it will be aligned with upcoming new framework features.

@taylorotwell
Copy link
Member

Can you do isset($model->foo)?

@geisi
Copy link
Contributor Author

geisi commented Oct 24, 2022

Isset indeed works for Attributes, GetMutators, Relations, and AttributeMutators. Sadly isset does not work for Castables.

@donnysim
Copy link
Contributor

Why would

if (array_key_exists($key, $this->casts)) {
    return true;
}

mean the attribute exists? This will result in always returning true because a cast exists even though the attribute wasn't selected - doesn't exist.

@ghost
Copy link

ghost commented Oct 26, 2022

@donnysim I guess if a casts exists, the model must has that attribute at some point. Even if that means that that the attribute is null or still has no allocated value at all.

@donnysim
Copy link
Contributor

But isn't the whole point of hasAttribute to check if it exists now not if it will exist at some point? Because what does it actually solve then? It will say true and the next thing you get will be a missing attribute exception because it lied?

@geisi
Copy link
Contributor Author

geisi commented Oct 26, 2022

If there is a cast no missingAttributException will be thrown. The hasAttribute() method abstracts the checks which are done previously done in the getAttribute() method. So if hasAttribute() is true you can be sure that no missingAttributeException will be thrown.

@donnysim
Copy link
Contributor

Oh, so the whole attribute strict mode is even more crooked than I imagined 😂. It shouldn't matter if a cast is defined or not, if the attribute does not exist for casting it should throw an error. Isn't it the whole point of it?

class TestModel extends Model
{
    protected $casts = [
        'parent_id' => 'int',
    ];
}

TetsModel::first(['id'])->parent_id // <- should throw not silently return null

Sadly the more I follow the whole strict mode changes the more I'm inclined to just never use the built in way because it's a minefield in itself, so I'll just leave you do your things and carry on.

@geisi
Copy link
Contributor Author

geisi commented Oct 26, 2022

This PR is not a general discussion regarding Laravel's strict mode.
But in my opinion, it's the right way how it is currently implemented. The only reason when a ModelAttributeMissing Exception should be thrown is when the attribute is not defined anywhere (Database, Casts, GetMutators, Relations ...). And this is what the current implementation does.

@inxilpro
Copy link
Contributor

Personally, I think that hasAttribute would have a place if #44642 hadn't been merged. My preference would be that we revert #44642 as it complicates the semantics of preventAccessingMissingAttributes and add a hasAttribute method that essentially behaves like the current offsetExists implementation.

Either way, I don't think hasAttribute() should return true just because a cast exists. And if we don't revert #44642, I think the current offsetExists behavior covers this use-case.

@taylorotwell
Copy link
Member

I honestly am not sure I plan to mess with this anymore. I'm not super happy with how "strict mode" has panned out and it has felt a bit more trouble than it's worth.

@royduin
Copy link
Contributor

royduin commented Jun 25, 2024

It's added in Laravel 11.3: #50909

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

Successfully merging this pull request may close these issues.

5 participants