Dont call identifiable attribute in models that dont use crud trait

[pxpm]

refs #3635

If the related model does not use CrudTrait, identifiableAttribute will not be available to call. It's developer responsability to setup attribute or return the predefined options.

This is just a small change, it would error anyway if method don't exist on model.

[nbolender]

@pxpm This makes sense to me and would solve my issue from #3635, while still automatically calling identifiableAttribute if defined by the developer. Thank you!

[tabacitu]

Wouldn't this be the same as

if (isset($field['model']) && ! isset($field['attribute']) && method_exists($field['model'], 'identifiableAttribute'))

? One if statement instead of two?

[tabacitu]

Or maybe it makes more sense to keep the separate if, but give it a raison d'être 😅 Would it be a better UX if identifiableAttribute doesn't exist to show a custom error message, and tell the developer that model X doesn't have identifiableAttribute() so it's probably not using CrudTrait and they need to do that? Maybe it's an opportunity to turn a negative experience into something... less negative 😅

What do you think @pxpm ?

[pxpm]

Sure, it's totally fine in just one if statement, I made it two for readability purposes, maybe I was wrong.

I do not agree we should raise an exception here so there is no raison d'être voilá merci ça va 😺 for that second if. I will refactor after we brainstorm on this.

The intent of this change, is to use relational fields with Models that don't use crud trait, for example in an select2_from_array, that is a relation, but you are providing the final options ($key => $value) to the field, so you might be using that Model for the specific purpose of holding the options, but without the need of adding crud trait.

It would make sense in, let's say a relationship field, because there we need attribute or the related model to have CrudTrait so we can infer it.

What we could do to avoid the specific scenario of select_from_array, is in the field initialization cycle, we check if options are present, if they are we don't infer the attribute ? We assume that the options are final key => attribute for the field ?

This last way would allow us to do what you said about raising the exception when really dev. messed up and forgot to add CrudTrait.

Let me know,
Pedro

[tabacitu]

The intent of this change, is to use relational fields with Models that don't use crud trait

Ooooh! I get it now, ok.

What we could do to avoid the specific scenario of select_from_array, is in the field initialization cycle, we check if options are present, if they are we don't infer the attribute ? We assume that the options are final key => attribute for the field ?

Huh... that sounds smart. But do we even get the identifiableAttribute if options is already defined? I don't think so.

This last way would allow us to do what you said about raising the exception when really dev. messed up and forgot to add CrudTrait.

Yeah no, sounds like I was overthinking this one too. Let's not throw an error in this case. We're not sure it's an error - the dev might have chosen to point to a non-CrudTrait model. And we're fine with that.

Sorry 🙏 But yeah let's merge the two ifs into one please.

[promatik]

@pxpm I merged both if statements as Tabacitu suggested ✌

[scrutinizer-notifier]

The inspection completed: No new issues

[pxpm]

Thanks @promatik \o

Good to go!