Skip to content

Conversation

@ipundit
Copy link
Contributor

@ipundit ipundit commented Sep 3, 2023

shmax clarified the expected return values of Model::find() dependent on its input. See #37. This is confusing behavior for a function, but to change it would lead to breaking changes on users of this base library.

This PR does the next best thing, which is to clarify the documentation in the function header comments and also in the test cases.

Also did some cleanup of the .gitignore file. Usually package-lock.json would be source controlled so that you know which versions of dependencies work and when you'd have to do a composer update after a pull, but since composer.lock was ignored, I've also ignored package-lock.json.

ipundit added 17 commits August 31, 2023 18:10
…at capitalization for attribute names are retained. So the short form for New York is now NY instead of ny. test/model/Author, Book, and Venue now have those strtoupper() workarounds in getter/setter methods removed as it's done in Model.php
…rcased in Model.php in order to be case insensitive.
…at capitalization for attribute names are retained. So the short form for New York is now NY instead of ny. test/model/Author, Book, and Venue now have those strtoupper() workarounds in getter/setter methods removed as it's done in Model.php
…rcased in Model.php in order to be case insensitive.
…cord

# Conflicts:
#	test/ActiveRecordTest.php
#	test/models/Book.php
…Disable it so that regression does not timeout.
…d#35:

Authors table now has a column that is camel cased: firstName
Added tests that demonstrate case insensitivity when accessing this firstName column
- Correct link to contributing.md
- Added test case to demonstrate how to use a select of specific columns on equality to a primary key
- Added test case to demonstrate that find("conditions") will always return an array, even if ['limit'=>1]
@shmax
Copy link
Contributor

shmax commented Sep 3, 2023

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?

@ipundit
Copy link
Contributor Author

ipundit commented Sep 3, 2023

Would this be good to merge in now and then later decide on what to do for #45, and then develop a PR for that? This PR is designed to not be breaking or change the method signature in any way. Whatever is decided for #45 would likely do that.

@shmax shmax merged commit 0fdf09c into php-activerecord:master Sep 3, 2023
@shmax
Copy link
Contributor

shmax commented Sep 3, 2023

Nice, thank you!

@ipundit ipundit deleted the FindDocumentation branch September 5, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants