Skip to content

Conversation

pine3ree
Copy link

As in

$posts = Post::findAllByCategory('music');
//..
$user = \models\User::findByEmail('user@example.com');
//..
echo $post->toJSON();

As in
```
\models\User::findByEmail();
$post->toJSON();
```
@koenpunt
Copy link
Collaborator

koenpunt commented Dec 6, 2014

Needs tests. And why would anyone use the camelCased version of methods, when that in the end performs worse (not sure, only assuming that passing a method call to call_user_func_array is less performant)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why even have this method_exists() check block?
This won't allow for the build or create methods and this __call method already handles the error case if a method doesn't exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same goes for the static version above.

@pine3ree
Copy link
Author

Everything works as before: the only addon feature is that a call to a nonexistent method:

findAllByCategory

will be translated to a call to:

find_all_by_category

  1. if a snake_case method named "find_all_by_category" exists => it will be called directlly by call_user_func_array

  2. otherwise the default php-activerecord method resolution occurs.

....nothing fancy, just allowing cameCase method calls

"And why would anyone use the camelCased version of methods"

i just find camelCase more readable (and modern). You don't call finders 1000 times per request, I hope no one even call them 100 times per request, so performance will not be affected even if call_user_func_array is slower.

also any magic method call is slower than callign an existing method, but that is how php-activerecord magic finders are implemented. Again the numer of calls per request make this negligible.

kindly

@koenpunt
Copy link
Collaborator

I understand what it does, but to verify the functionality, and to be sure that it won't break in the future there need to be tests

@cvanschalkwijk
Copy link
Collaborator

I think a lot of developers are switching over to this convention and it would be nice to allow it in the finders. I agree that performance impact would be trivial if any from method_exist lookups. Could you include a test which includes using your camelCased finders if you want us to merge this in.

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.

4 participants