Skip to content

Conversation

@d13r
Copy link
Contributor

@d13r d13r commented Dec 13, 2014

Currently this code, with snake-casing, doesn't use the eager-loaded data - it runs the database query again:

$contacts = Contact::with('emailAddresses')->get();
$email_addresses = $contact[0]->email_addresses;

Whereas this code, with camel-casing, correctly uses it:

$contacts = Contact::with('emailAddresses')->get();
$email_addresses = $contact[0]->emailAddresses;

This PR fixes it so it will always use the camel cased version in the array key, so that both spellings will return the same object.

So this code correctly uses the pre-loaded relation instead of running
another query:

$contacts = Contact::with('emailAddresses')->get();
$email_addresses = $contact[0]->email_addresses;
@GrahamCampbell
Copy link
Collaborator

I don't know if this was an intentional behaviour, so this might have to go into 5.0. // cc @taylorotwell.

@GrahamCampbell GrahamCampbell changed the title [4.2] Fix handling of camel case relation attributes [4.2] Fix Handling Of Camelcase Relation Attributes Dec 13, 2014
@d13r d13r changed the title [4.2] Fix Handling Of Camelcase Relation Attributes [4.2] Fix Handling Of CamelCase Relation Attributes Dec 13, 2014
@taylorotwell
Copy link
Member

Yep, you always access relations as they are defined.

@d13r
Copy link
Contributor Author

d13r commented Dec 16, 2014

  1. This isn't documented anywhere.
  2. The code calls camel_case() lower down which means both versions already work but only one version is correctly cached, making it really easy to write inefficient code, and really hard to debug.

@d13r
Copy link
Contributor Author

d13r commented Dec 16, 2014

I feel I need to try to explain this better, because I honestly can't see @taylorotwell's logic for not merging this. But maybe I'm missing something...

Laravel already supports snake-cased attributes for both columns and relations. For columns it's even required - e.g. $user->firstName doesn't return the first_name column:

[1] > $x = App\User::find(1);
// object(App\User)(
...
// )
[2] > $user->first_name;
// 'Dave'
[3] > $user->firstName;
// NULL

For relations Laravel already explicitly calls camel_case(), which as far as I can tell is only necessary to support snake case relations like this - so if you don't want support snake casing relation names why call it?

The docs don't seem to offer any opinion about which should be used - camel case or snake case - because all the examples use single words (phone, user, comments). So I went with what I think is the logical option - to be consistent between relation and column names and always use snake case.

Now in 99% of cases this has worked fine, because Laravel does convert back and forth automatically. Except in this 1 particular case it breaks. Except it doesn't actually break - it works fine, but inefficiently - with 100 database queries (one per row) instead of 1 - and it's only because of Debugbar that I know this. But even knowing that it took me a while to debug it.

So, while using camel case for this one purpose is a possible workaround, that leaves me with inconsistent code - I have to remember to use snake case for columns and camel case for relations - and as I said above, why support snake case relations at all if we shouldn't be using them?

Of course if changing it broke something that would be different, but that wasn't the reason given, and I can't think of anything that it would break. As Graham suggested, I'd be happy to move this across to 5.0 if there were concerns about breaking something. (In fact I originally fixed it there, but then I back-ported it because I consider it to be a bug and you explicitly say "Bug fixes should never be sent to the master branch".)

Thank you for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants