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

Make Table::findOrCreate() more flexible #8671

Closed
2 of 3 tasks
burzum opened this issue Apr 18, 2016 · 6 comments
Closed
2 of 3 tasks

Make Table::findOrCreate() more flexible #8671

burzum opened this issue Apr 18, 2016 · 6 comments
Assignees
Milestone

Comments

@burzum
Copy link
Contributor

burzum commented Apr 18, 2016

This is a (multiple allowed):

  • bug
  • enhancement
  • feature-discussion (RFC)
  • CakePHP Version: 3.x

What you did

Wanted to implement findOrCreate() and realized it is limited on customizing the find() call.

Expected Behavior

I would like to be able to define my own finder or query object.

Actual Behavior

I can only pass conditions, not define contains or use a finder as first arg.

Suggestion

Allow the first arg to be a query object or a string that is used as a finder.

The only problem I see is that the method has just 2 args for now, but it would be useful to have another one to pass something to the options of the finder.

@burzum burzum self-assigned this Apr 18, 2016
@burzum burzum added this to the 3.3.0 milestone Apr 18, 2016
@lorenzo
Copy link
Member

lorenzo commented Apr 18, 2016

Would you like to submit a pull request for this?

@burzum
Copy link
Contributor Author

burzum commented Apr 19, 2016

@lorenzo I can do that, but you know that I'll never write code before discussing it to avoid wasting my time with something that is not accepted at the end. 😄

So let's discuss how to implement this right. My main concern right now is how to keep it BC and allow all three proposed solutions. I think changing the method signature and checking for the kind of the first passed argument could make this possible.

  • findOrCreate($search, callable $callback = null)
  • findOrCreate($queryObject, $search, callable $callback = null)
  • findOrCreate($finderName, $finderOptions, $search, callable $callback = null)

Any better or additional ideas?

@markstory
Copy link
Member

Isn't a query object 'better' than a finder?

$articles->findOrCreate($articles->find('published'), function () { ... });

@htstudios
Copy link
Contributor

  • what about a 'orCreate()' that picks up a query object?
  • why is it named findOrCreate when it implies ->first() - should it be getOrCreate()?
  • finder option is consistent with how get() work, right?

@burzum
Copy link
Contributor Author

burzum commented Apr 21, 2016

@markstory I'm OK with just the query object as well, just one question: How do we pass the search conditions then? Maybe like this?

findOrCreate($queryObject, $search, callable $callback = null)

And make the search optional? The problem I ran into in my case was that the $search is used for the entity / save as well but that I had to prefix the fields with the alias, which then made the entity fail. So I ended up with doing this in my case.

        $project = $this->Projects->findOrCreateProject([
            'Projects.is_draft' => true,
            'Projects.company_id' => $companyId,
            'Projects.profile_id' => $profileId
        ], function($entity) use($companyId, $profileId) {
            $entity->name = '';
            $entity->is_draft = true;
            $entity->profile_id = $profileId;
            $entity->company_id = $companyId;
        });

Edit: I ended up implementing the string check for search as well but a little different. Instead of checking for a finder with that name I'm checking for method exists. This allows us to keep the actual code and method that will return the query in the model. See the PR.

@markstory
Copy link
Member

Closing as the proposed changes are in a pull request form.

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

No branches or pull requests

4 participants