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

[NFR]: Phalcon\Mvc\Model::_invokeFinder should merge provided arguments #14573

Closed
CameronHall opened this issue Dec 2, 2019 · 3 comments
Closed
Labels
new feature request Planned Feature or New Feature Request

Comments

@CameronHall
Copy link
Contributor

Is your feature request related to a problem? Please describe.
You should be able to provide arguments to findFirstBy and findFirst. I.e.

MyModel::findFirstByName('poato', [
    'order' => 'id DESC',
]);

Describe the solution you'd like
To be able to provide the argument and have them work...

Describe alternatives you've considered
I was going to resort to overloading _invokeFinder but the effort to reward was not worth it - considering I'd also be rewriting a method in userland vs. Zephir/C. Oh, and it's final.

@CameronHall CameronHall added the new feature request Planned Feature or New Feature Request label Dec 2, 2019
@niden niden added the 4.0 label Dec 4, 2019
@niden niden mentioned this issue Dec 4, 2019
5 tasks
niden added a commit that referenced this issue Dec 4, 2019
niden added a commit that referenced this issue Dec 4, 2019
@niden
Copy link
Member

niden commented Dec 4, 2019

Implemented in #14580

@niden niden closed this as completed Dec 4, 2019
@CameronHall
Copy link
Contributor Author

Appreciate this one getting done so promptly, I think this should have also given us the ability to specify secondary arguments. For example;

MyModel::findFirstByName('poato', [
   'conditions' => 'status = :status:',
    'bind' => [
        'status' =>MyModel::STATUS_ACTIVE,
    ]
    'order' => 'id DESC',
]);
// OR
MyModel::findFirstByName('poato', [
   'status = :status:',
    'bind' => [
        'status' =>MyModel::STATUS_ACTIVE,
    ]
    'order' => 'id DESC',
]);

Then the conditions will be joined with an AND (<second condition>). It's really syntaxical sugar, but I feel like its a neat little time saver and the function call defines the intent.

What do you think @niden, @ruudboon ?

@niden
Copy link
Member

niden commented Dec 4, 2019

Well it kind of defeats the findBy<field> objective. If you have more than one field you can use find and since you are defining conditions in your array, then one more (for the by) does not use much time. This is why I did not add more into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature request Planned Feature or New Feature Request
Projects
None yet
Development

No branches or pull requests

2 participants