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

[5.5] Add hasAttribute to Eloquent/Model #22250

Closed
wants to merge 3 commits into from

Conversation

johannesschobel
Copy link

This PR adds the hasAttribute() method to Eloquent/Model in order to check, if a given attribute exist in this model.

Why do we need this method?! Because other "checks", like isset(...) show a wrong behaviour.

Consider the following example here:

// let $user be an instance of Eloquent/Model
$entity->name = null;
dd(isset($entity->name)); // false, even it is set!
dd($entity->hasAttribute('name')); // true, even it is null

Furthermore, property_exists(...) does not work either...

So this would be a valuable contribution...

This PR adds the `hasAttribute()` method to check, if a given attribute exist in this model
*/
protected function hasAttribute($key)
{
return array_key_exists($key, $this->attributes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work on mutated attributes? You should probably be using attributesToArray() instead of the attributes property.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure on this.. because this method only checks if this key (i.e., the field) is set.. The value is not needed for this method..

As far as i know, mutators should only work on attributes (e.g., apply additional functions on the values in order to change formatting or whatever). Therefore, $this->attributes should be enough for the scope of this function..

Or am I missing something really critical?!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite true, although mutators should have a relationship with attributes someone could create a totally independent mutator.

But what I meant in the first place is, no mater if the attribute is a mutator or not this should return true, if not it could lead to confusion as I do not need to know if the attribute was a mutator in the first place or a real attribute.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i changed it to attributesToArray() - thank you for pointing that out..

@devcircus
Copy link
Contributor

Seems this is not consistent with other methods such as getAttribute($key) that does consider mutators.
Just seems unnecessary anyway.

@tillkruss tillkruss changed the title Add hasAttribute to Model [5.5] Nov 30, 2017
@johannesschobel johannesschobel changed the title [5.5] [5.5] Add hasAttribute to Eloquent/Model Nov 30, 2017
Use `attributesToArray()` instead of `attributes`
@johannesschobel
Copy link
Author

@tillkruss : i changed the name of the PR.. actually your rename-operation removed everything ;)

@devcircus : Currently, there is no way of checking, if a given $key is an attribute of the model. Therefore, i think, this might be a valuable contribution. The "solution" @taylorotwell mentioned in #22249 ("use isset(...)) is not appropriate, as pointed out by @decadence ...

@jmarcher : i changed it to use attributesToArray() as you suggested...

@jmarcher
Copy link
Contributor

Perfect, looks good to me

@DrowningElysium
Copy link
Contributor

Couldn't you fix the offsetExists method that is used inside __isset()?
This should also fix the isset() helper

http://php.net/manual/en/language.oop5.overloading.php#object.isset

And when you have that fixed you kinda could use hasAttribute as an alias method

@johannesschobel
Copy link
Author

@DrowningElysium wouldn't that be way to much magic for this use-case here?!

@DrowningElysium
Copy link
Contributor

Well the PR seemed like provide an alternative for functions that are broken.

So I thought it would be better to fix at least a bit of the issue and have the alternative for the property_exists function

Seems like we are now building an alternative instead of fixing the issue we are having.

@johannesschobel
Copy link
Author

the problem with "overloading" the isset(..) function is - at least for me - that people wouldn't be aware of the fact, that the isset(...) behaves differently when called on a Model class..

Furthermore, i think, calling $model->hasAttribute('name') is much more readable than calling isset($model->name)...

@devcircus
Copy link
Contributor

Now that you're checking attributesToArray, at least it is consistent with getAttribute. Still not convinced it is necessary, but maybe it's helpful to some.

@johannesschobel
Copy link
Author

@devcircus : currently, i have the use-case where i explicitly need to check, if a specific attribute exist in a model - and behave differently, if it does not.. So for certain use-cases it might be "important" to have such a helper..

@johannesschobel
Copy link
Author

any news on this PR here, @taylorotwell and community?
keep up the good work.. thank you very much for your effort...

@taylorotwell
Copy link
Member

taylorotwell commented Dec 7, 2017

array_key_exists($key, $model->attributesToArray()) or array_key_exists($key, $model->getAttributes()) can already be used.

@johannesschobel
Copy link
Author

Hey @taylorotwell and @GrahamCampbell ..

My PR actually is exactly this line of code you suggested. However, always writing array_key_exists(...) is cumbersome and ugly. Therefore, I provided this "helper" method in order to provide an easy-to-use method..

Then why even provide any new functions to laravel? Actually you can already use "built-in" functions from SPL in order to achieve whatever you want..

oh wait, i forgot.. SPL is quite ugly to use and "easy-to-use" helper functions are appreciated by developers..

So can you please re-open and merge this PR? Thank you very much!

@solicomo
Copy link

solicomo commented Jan 9, 2020

array_key_exists($key, $model->attributesToArray()) or array_key_exists($key, $model->getAttributes()) can already be used.

This won't work if $model is a new instance just created.
And if there is no record in table, we will not be able to get an instance that has all the attributes.

In this case, hasAttribute() will be valuable. It should fetch all the column names at the first time it is called.

And I'd like to see a static getAttributes() function. So I don't have to create an instance to get all the column names.

@fractalf
Copy link

fractalf commented Jun 7, 2022

I cannot believe this was rejected :(
This would be so usefull
Shame...

@vassilidev
Copy link

Totally ridiculous, shoud'nt be rejected

@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.

9 participants