With this change one won't be able to stack finder calls, that's sad.

Let's say one make's a custom finder findCustom(Query $query, Entity), you can't do:

$table->find('all', ['conditions' => '..'])->find('custom', $someEntity);

@ADmad why not? What makes it impossible with this implementation?

NVM, for some reason I remembered that the options used for 1st find call are passed down to subsequent calls but that's not the case.

We will have to add clarification to the docs that if 2nd argument for find() is array it will always be treated as options for query. Still lot of people will overlook this quirkiness 🙂.

We will have to add clarification to the docs that if 2nd argument for find() is array it will always be treated as options for query.

That's my main concern with this.

I was wondering that maybe we should remove this part: https://github.com/cakephp/cakephp/pull/12787/files#diff-d36d5650ddbf69668bdd04f31f4a7a42R2217

And set options on query explicitly in build-in finders that support options array. I imagine that this change would stay BC but would require people to update those custom finders that for some reason expect that the options are set on query (even though those still should be accessible from the argument).

...but would require people to update those custom finders...

I am not in favor of any change which requires updating custom finders. 4.0 is supposed to a be smooth upgrade from 3.x and finders are a very important and well used feature of the framework.

Does it still make sense, can I continue with that?

I dont like the change tbh.

Why not?

Let me ask this:

• How do static analyzers work with this change compared to array with documented types of keys? Psalm for example can introspect even the types on those assoc arrays
• How is the usage for IDEs here (for developers)? Can this easily be understood here on what arguments are available and how many are used etc?

@dereuromark I'm not able to answer your questions but let it be clear: this is not a breaking change. You can still use options array as before. This change allows you to pass anything to the find() method after a finder name.

For me personally being able to leverage PHP 7 type checking instead doing it manually in every finder would be a great feature and I'd be willing to sacrifice a bit of code discoverability. But this change does not force you to write finders the new way, it just opens a path for being able to write less error-prone code.

ping @robertpustulka
Do you still want to land this into 4.x?

Yes. Does it look OK as it is or do we need another approach? I'd like to know before I fix conflicts and add tests.

I like the idea of this, but the interface is awkward now.

I would prefer to see query options pulled out as a separate parameter. Treating the first argument in a variable list as the query options parameter causes a lot of one-off and note style documentation that is hard to read.

The fact that they were merged with other general table-specific finder options is more or less a miracle that just happens to work and feels nicely hidden because everyone can just read examples or docs of a single array argument ... there's never a special case where someone reads an example without an options array as the first argument.

@othercorey

I would prefer to see query options pulled out as a separate parameter. Treating the first argument in a variable list as the query options parameter causes a lot of one-off and note style documentation that is hard to read.

I'm not sure I understand you, but you can pass anything to the query with the proposed interface. The first argument is treated as query options by default only for the BC.

Eg:

$this->find('foo', ['foo' => 'bar']); //the BC way
$this->find('bar', $foo, $bar); // $foo and $bar ca be anything and get passed to the finder as below:

public function findBar($query, $foo, $bar) {}

The arrayish options can still be pulled from the query object with $query->getOptions().

I agree that the interface in the 3.x was a bit hacky with mixed finder and general options but this have been done to keep the BC with 2.x, where you called finders like:

$this->Articles->find('all', [
    'conditons' => [...],
]);

Maybe it's too late now to do this for 4.x, but maybe with 5.0 we should remove the "special" options that are proxied to the query methods (like conditions, fields etc)?

I'm not sure I understand you, but you can pass anything to the query with the proposed interface. The first argument is treated as query options by default only for the BC.

I'm saying the backwards compatible part is what makes this extremely awkward.

@othercorey OK. Do you have an idea how to handle this more conveniently?

@othercorey OK. Do you have an idea how to handle this more conveniently?

I would require calling a reserved findXXX() method with an options parameter to start the find chain.

findByArray($options)->find('custom', $param1, $param2)

• findByArray() should be a declared method, not a magic method.
• Require the $options array passed to findByArray() to only contain supported keys and throw a deprecation warning when it contains more.
• Require $param1 to not contain the findByArray() $option keys and throw a deprecation warning when it contains them.

This will enforce splitting the query options from find parameters while letting the user know they need to upgrade.

I'd prefer to just throw exceptions, but depending when the change is made, you can throw deprecation warning.

With this solution you'd need to locate and replace all find('all', [..]) calls in existing code with findByArray(). I'm afraid that would be an overkill.

I have another idea - maybe we should have a new find method (both on repository and query) to support the new API? This way we could still use the old API with option array and the new one with variadic arguments. This is can be introduced in a minor release without breaking people's code.

The new method could be named findBy so this stays in line with old findByFieldName convention?

$this->findBy('foo', $object1, $object2)->find('bar', $array);

How is that any different? The finder names will conflict with existing methods.

How would they conflict?

With this solution you'd need to locate and replace all find('all', [..]) calls in existing code with findByArray(). I'm afraid that would be an overkill.

Only for those that pass in query options.

Only for those that pass in query options.

Yes, but that is still a big breaking change in all community plugins and userland code. Please note that this applies not only to the pre 3.x find calls with conditions etc options but to all finders including custom ones that use option arrays.

Yes, but that is still a big breaking change in all community plugins and userland code. Please note that this applies not only to the pre 3.x find calls with conditions etc options but to the all finders including custom ones that use option arrays.

Well, you can fix the mess or not, lol.

I don't think that it's a mess. I don't even think that option arrays are inconvenience, because a lot of people like "named" arguments for options. With #12784 I just wanted to introduce more convenient way of passing objects into finders while leveraging built in PHP7.1+ type checking.

Just offering my opinion that mixing query options + custom finder options in an array that's treated as a special parameter in variadic magic finder methods is really awkward.

But, I it isn't up to me.

OK I see that's awkward but it had to be done one way or the other to keep the BC. But now I realise that the variadic parameter support can be introduced as a new method without breaking the existing API and the BC.

I'll come up with a new PR soon but this will probably target 4.1 now.

@ADmad please remind me - we can still introduce new method to interfaces in minors with 4.x?

Usually you would soft add new methods through docblock.

please remind me - we can still introduce new method to interfaces in minors with 4.x?

We can't update the interface in minor. But we could add the new method to Table class and add annotation to the RepositoryInterface as a compromise.

@robertpustulka Instead of making find() (or a new method) support variadic args why not just add a findX() method to your table/behavior with the arguments you want? That would also allow you to properly typehint the args.

While Cake conventionally uses methods named like findX() as custom finder and expects a specific signature, it doesn't really matter if you don't try to use your custom methods as finders. Or you can simply chose to not use find prefix.

I can’t chain finders using custom method.

True that. If only the ORM required Query::all() to be called explicitly, then Query::__call() could be changed to proxy to repository methods instead of resultset decorator methods 🙂.

I didn't mean for this to get closed, I was removing the now stale 4.x branch. If you could open the pull request against master that would be appreciated as I cannot change the base branch now.