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

Contain usage of custom finder not working #10223

Closed
thinkingmedia opened this Issue Feb 14, 2017 · 15 comments

Comments

Projects
None yet
6 participants
@thinkingmedia
Member

thinkingmedia commented Feb 14, 2017

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • CakePHP Version: 3.4.0

  • Platform and Target: Apache 2, PHP 7.1.1, MySQL 5.7

What you did

        $jobs = $this->Jobs->find('active')
            ->contain(['JobLogs' => [
                'strategy' => 'select',
                'finder' => ['orphan' => ['node_id' => $nc->node_id()]]
            ]]);

Jobs is returned with all JobLogs records.

What happened

Confirmed that finder is not called.

    public function findOrphan(Query $query, array $options = [])
    {
        dd('findOrphan');
    }

What you expected to happen

Finder for contain should be used instead of find "All".

@thinkingmedia thinkingmedia added this to the 3.4.1 milestone Feb 14, 2017

@thinkingmedia thinkingmedia self-assigned this Feb 14, 2017

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Feb 14, 2017

Member

I never knew such an usage was possible. Is it documented or are there test cases for it?

Member

ADmad commented Feb 14, 2017

I never knew such an usage was possible. Is it documented or are there test cases for it?

@thinkingmedia

This comment has been minimized.

Show comment
Hide comment
@thinkingmedia

thinkingmedia Feb 14, 2017

Member

@ADmad I'm digging in the code now. There's no support for this in the eagerLoader for 3.4, and that's okay. I'll see if I can figure out why I did it this way in 3.3.

Member

thinkingmedia commented Feb 14, 2017

@ADmad I'm digging in the code now. There's no support for this in the eagerLoader for 3.4, and that's okay. I'll see if I can figure out why I did it this way in 3.3.

@thinkingmedia

This comment has been minimized.

Show comment
Hide comment

@thinkingmedia thinkingmedia added Enhancement RFC and removed Defect labels Feb 14, 2017

@thinkingmedia

This comment has been minimized.

Show comment
Hide comment
@thinkingmedia

thinkingmedia Feb 14, 2017

Member

This was not a defect. It was just a side effect of how 3.3 loaded associations.

To move forward:

  1. Do we add support for this?
  2. Do we throw an argument exception saying "finder" is not support.
  3. Do we do nothing.
Member

thinkingmedia commented Feb 14, 2017

This was not a defect. It was just a side effect of how 3.3 loaded associations.

To move forward:

  1. Do we add support for this?
  2. Do we throw an argument exception saying "finder" is not support.
  3. Do we do nothing.

@thinkingmedia thinkingmedia removed this from the 3.4.1 milestone Feb 14, 2017

@robertpustulka

This comment has been minimized.

Show comment
Hide comment
@robertpustulka

robertpustulka Feb 14, 2017

Member
'finder' => ['orphan' => ['node_id' => $nc->node_id()]]

I like that. This way we could configure several finders.

Member

robertpustulka commented Feb 14, 2017

'finder' => ['orphan' => ['node_id' => $nc->node_id()]]

I like that. This way we could configure several finders.

@markstory markstory added this to the 3.5.0 milestone Feb 14, 2017

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Feb 14, 2017

Member

Do we throw an argument exception saying "finder" is not support.

We don't throw exceptions in other places when unexpected keys are provided in options arrays. I don't know why this would be different.

Adding support for finder sounds interesting. Resolving all the joins might get hairy as custom finders can modify the query with more 'contain' and 'matching' calls.

Member

markstory commented Feb 14, 2017

Do we throw an argument exception saying "finder" is not support.

We don't throw exceptions in other places when unexpected keys are provided in options arrays. I don't know why this would be different.

Adding support for finder sounds interesting. Resolving all the joins might get hairy as custom finders can modify the query with more 'contain' and 'matching' calls.

@thinkingmedia

This comment has been minimized.

Show comment
Hide comment
@thinkingmedia

thinkingmedia Feb 15, 2017

Member

@markstory we already support custom finders for associations, and in contain using the query builder. This is just a matter of syntax for using a finder.

Add the finder key to the $options array would be consistent with how Table::get() works with custom finders. That is the only other place that I could find that uses the key finder in options.

Member

thinkingmedia commented Feb 15, 2017

@markstory we already support custom finders for associations, and in contain using the query builder. This is just a matter of syntax for using a finder.

Add the finder key to the $options array would be consistent with how Table::get() works with custom finders. That is the only other place that I could find that uses the key finder in options.

@robertpustulka

This comment has been minimized.

Show comment
Hide comment
@robertpustulka

robertpustulka Feb 15, 2017

Member

@thinkingmedia @lorenzo

Is there anywhere support for multiple finders? Somethink like:

'finder' => ['foo' ,'bar']
//or
'finder' => [
    'foo' => [
        'id' => 1
    ],
    'bar'
]
Member

robertpustulka commented Feb 15, 2017

@thinkingmedia @lorenzo

Is there anywhere support for multiple finders? Somethink like:

'finder' => ['foo' ,'bar']
//or
'finder' => [
    'foo' => [
        'id' => 1
    ],
    'bar'
]
@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Feb 15, 2017

Member

The finder option for arrays in contain is called queryBuilder as explained in the API docs https://api.cakephp.org/3.4/class-Cake.ORM.Query.html#_contain

Member

lorenzo commented Feb 15, 2017

The finder option for arrays in contain is called queryBuilder as explained in the API docs https://api.cakephp.org/3.4/class-Cake.ORM.Query.html#_contain

@lorenzo lorenzo closed this Feb 15, 2017

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Feb 15, 2017

Member

I'm closing because I think there are already enough options for contain, no need to make it more complex than it already is. Just pass any callable to queryBuilder!

Member

lorenzo commented Feb 15, 2017

I'm closing because I think there are already enough options for contain, no need to make it more complex than it already is. Just pass any callable to queryBuilder!

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Feb 15, 2017

Member

Digging further In the code I can see the docs for finder where added by the author of that stackoverflow response, and that there is one shy test in the code to support it's usage. Given that I don't remember adding that, and it was probably me who did, I think I'll just deprecate it.

The excess of alternative syntax makes it difficult to not break things when refactoring

Member

lorenzo commented Feb 15, 2017

Digging further In the code I can see the docs for finder where added by the author of that stackoverflow response, and that there is one shy test in the code to support it's usage. Given that I don't remember adding that, and it was probably me who did, I think I'll just deprecate it.

The excess of alternative syntax makes it difficult to not break things when refactoring

@thinkingmedia

This comment has been minimized.

Show comment
Hide comment
@thinkingmedia

thinkingmedia Feb 15, 2017

Member

@lorenzo thank you for showing restraint on this.

Sorry I opened it :)

Member

thinkingmedia commented Feb 15, 2017

@lorenzo thank you for showing restraint on this.

Sorry I opened it :)

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Feb 15, 2017

Member

Nothing to be sorry about @thinkinmedia

This showed a poorly documented and poorly tested part of the code, so it was valuable :)

Member

lorenzo commented Feb 15, 2017

Nothing to be sorry about @thinkinmedia

This showed a poorly documented and poorly tested part of the code, so it was valuable :)

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Jun 20, 2017

Member

@lorenzo I too stumbled upon this yesterday. One of the plugins I maintain has such usage which worked prior to 3.4 and fails now on 3.4.

The docblock for Query::contain() does show examples of using finder option and also there are tests for it. So if it doesn't work currently it's a regression IMO that should be fixed.

@thinkingmedia Do you still have problem with the example you have provided? Similar usage in the test case I linked also fails.

Member

ADmad commented Jun 20, 2017

@lorenzo I too stumbled upon this yesterday. One of the plugins I maintain has such usage which worked prior to 3.4 and fails now on 3.4.

The docblock for Query::contain() does show examples of using finder option and also there are tests for it. So if it doesn't work currently it's a regression IMO that should be fixed.

@thinkingmedia Do you still have problem with the example you have provided? Similar usage in the test case I linked also fails.

@cleptric

This comment has been minimized.

Show comment
Hide comment
@cleptric

cleptric Jun 21, 2017

Member

Closing again as we have an open PR to fix this.

Member

cleptric commented Jun 21, 2017

Closing again as we have an open PR to fix this.

@cleptric cleptric closed this Jun 21, 2017

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