> This is confusing behavior for a function, but to change it would lead to breaking changes on users of this base library.

[ipundit]

          > This is confusing behavior for a function, but to change it would lead to breaking changes on users of this base library.

Well, we're gearing up to do the first major version release in more than a decade, here, so if you want to break stuff now is the time to do it. For what it's worth there are a few other API-breaking changes that I felt were necessary for the benefit of static checking (namely, the various validates_on series of methods now expect an array indexed by field name, rather than a single crazy array with everything mixed-in together).

As for whether "conditions" should be a singular model or array of them, well, I have no idea.

I had to analyze all this in some depth when I was writing the PHPStan extension for Model::find, and it was very difficult to come up with a simple rule to govern all the cases I see in the find documentation.

My first thought was "ah, it's simple! If there's only one argument, then you only get one record back!". But that gets violated by this:

$books = Book::find([1, 3, 8]); // array

so I thought "oh, okay, well if there's only one argument but it's an array, then we go with array". But that gets contradicted by this:

$book = Book::find(['name'=>'Ubik']); // single

And so on. You clearly think that ["conditions"=> ...] should be a single, but based on what rule? I'm not saying you're wrong, I'm just curious to see your gears turning.

I think convention is going to have to drive all this, but I don't really know where to go for the source of truth. This library was launched over a decade ago and was modeled after Ruby's implementation of Active Record, so if you really want to help sort it all out you might see if you can make sense of their documentation:

https://guides.rubyonrails.org/active_record_basics.html

I actually find the docs kind of scattered and hard to read, so even better might be to get Ruby working locally and maybe play around with it.

edit: oh, and I forgot to mention; I wasn't able to find a "conditions"-style interface in the Ruby docs, so maybe it's not a thing, anymore?

Originally posted by @shmax in #42 (comment)

[ipundit]

If you're willing to break things, then I suggest splitting Model::find() into Model::find() and Model::all()/

// type is one of:
// int|string User::find(3);
// "first" User::find("first");
// usually used with FindOption User::find("first", ["name"=>"Waldo"]);
// "last" User::find("last");
// usually used with FindOption User::find("last", ["conditions"=>['name' => 'Waldo']]);
// options is a new class called FindOption where you can set select, joins, include, conditions, limit, offset, order, readonly, group in a type-checked way. It would also support ["name"=>"Waldo"] as an alias of ["conditions"=>['name' => 'Waldo']]
// @return A single row of data if found, or null if not. If multiple rows are found, emit a warning or throw an error telling the user to use Model::all() instead
// @throws nothing instead of RecordNotFound because it's covered by the null case
public static function find($type, $options: FindOptions = null): static|null

The type parameter
// array<string, int|string> User::find(["name"=>"Philip"]);
would either be no longer supported, or be an alias to User::find("first", ["name"=>"Philip"]);
as it was ambiguous as to exactly which Philip it would find since Model::find() as it is currently written would only return one row and there could be many of them.

// type is one of:
// blank User::all()
// null with FindOption User::all(null, ["name"=>"Stephen"])
// or User::all(null, ["conditions"=>["name"=>"Kurt"]]);
// ...int|string User::all(1, 3, 5, 8);
// array<int,int|string> User::all([1,3,5,8]);
// @return 0 or more rows of data
// @throws nothing because it returns [] instead of RecordNotFound
public static function all($type, $options: FindOptions = null): static[]

[shmax]

I'm going to experiment with Ruby a bit later this afternoon to see how its API compares with your notes, here.

By the way, I don't know if you're aware of this, but you can insert a code snippet in markdown by using ticks, so this:

```php
  // a comment
 $book = Book::find(1); 
```

...will render as

  // a comment
 $book = Book::find(1); 

[shmax]

I managed to get a Ruby on Rails test project running complete with debugger, so I'll log what I can glean about return types here.

Model.first

# single Model
User.first # Not sure I understand this 'no-arg method call`... what happens if there's already an attribute called 'first'?

# array of Model
User.first(3) # where the numeric argument is the number of items to return

Model.find

# single Model
User.find(15) 

# array of Model
User.find(157312430, 14035331)
User.find([157312430, 14035331])

And that's as fancy as pure find gets. If you want to limit on conditions, you can do this:

Person.where("administrator = 1").find(1)

What happens in the where doesn't affect the return type, that's still controlled by the find chained at the end.

In our PHP version of things, we don't have the chaining so if you want to do a "where" you need to pass a final $options argument to any call to find

Person::find(1,2,3, [ 'administrator'=>1]); // array<Person>

As for "conditions", I'm not sure it was ever really meant to be a part of the external API, and while it is currently used internally (by the magic find_by and count_by methods), I don't think it does anything externally that couldn't be accomplished by just passing that final argument:

  User::find(["conditions"=>["name"=>"Kurt"]]); 
   User::find(["name"=>"Kurt"]) // same

The only caveat there is that there is no equivalent find usage in Ruby on Rails, every call to find takes an id, or an array of ids (or a leading "all" argument).

There's more analysis to be done here, but I think for starters we're clear to remove "conditions" from the public API documentation, and I think we can also kill the plain Model::find($options) syntax in favor of Model::find("all", $options) or Model::all($options).

What do ya think?

[ipundit]

I figured out a way to support chaining in #49, so the following is now passing in CI:

$query = Author::select('authors.author_id, authors.name')
    ->orderBy('name DESC')
    ->limit(2)
    ->groupBy('name')
    ->offset(2)
    ->having('length(name) = 2')
    ->readonly(true)
    ->where(3);
$this->assertEquals(null, $query);

It doesn't touch the find() code in any way, so all those old tests pass. Unlike find, where() will always return a Model or null if not found.

$queries = Author::select('authors.author_id, authors.name')
    ->orderBy('name DESC')
    ->limit(2)
    ->groupBy('name')
    ->offset(2)
    ->having('length(name) = 2')
    ->readonly(true)
    ->all([3]);
$this->assertEquals(0, count($queries));

works too, and all the old ActiveRecordFind's all() tests still pass too. Unlike find, all() will always return a Model[], including a zero-length [] if no models are found

[shmax]

I'm dying to dive in on this stuff with you, and I think you're on the right track, but we really, really need to get the PHPStan checker cranked up to 8, as it will be critical in analyzing the correctness of what you're doing, here. I have around 32 errors left to fix at level 7, and then I have to start in on lvl 8. Would really love some help with a few issues I've noticed along the way, if you're keen to move forward:

• Model::$delegatehas the oldarray thing happening. Would like to convert it to array<string,DelegateOptions>, but I don't really understand what it does, and there don't really seem to be any tests around it. edit: also fixed
• Model::$__relationships can contain models or arrays of models, which is confusing the crap out of both myself and the static checker. Would be nice to normalize it to always use arrays. edit: cleaned it up

Let me know if you're interested before you dive in so we don't wind up stepping on each other's toes.

[ipundit]

See PR #63 for the conclusion to this issue.