Fixed select(), implemented pluck(), ids(), reselect(), none() #69

ipundit · 2023-09-14T05:03:37Z

Fixed bugs in select() related to repeated column names. See RelationTest::testSelectChainedSelects() for more details.

Implemented magic getters for Relation->last, first, and take so that you don't have to use parentheses like $relation->last(). Ruby on Rails supports this syntax, and now so do we.

Unfortunately, Model::last cannot be implemented because PHP has not implemented a __getStatic($name) magic function yet, only __get($name), so this is one example where we cannot by fully compliant to the RoR spec because of differences between PHP and Ruby.

…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.

# Conflicts: # lib/Model.php

…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

…gres regression

- Correct link to contributing.md

…vity() is enough.

…cord

…cord # Conflicts: # composer.json # scripts/TestCommand.php

…cord

…cord into FixWarning

…cord

…cord into RelationMagicFunctionsAndSelectFix

lib/Model.php

lib/Relation.php

shmax · 2023-09-14T05:47:10Z

test/RelationTest.php

+
+    public function testSelectChainedSelects()
+    {
+        $relation = Author::select('name,name')->select('author_id');


Looking at this test, it's not clear to me what the bug is you're fixing. I'm not saying you're wrong, just that it's not clear. For starters, using reflection to dip into internals as proof that there is a problem is sort of breaking the rules. If you want to show a problem, do it with publicly available means. For instance:

$author = Author::select('name,name')->take(); $this->assertEquals(['name' => 'Tito'], $author->attributes() );

This is why I suggested starting with a failing test case for us to look at before diving in. Recommend starting a new branch that has a single failing test case, preferably performed without the kind of introspection you're doing here. Once we understand the situation better, we can talk about what the fix is.

Hopefully the new commit explains things better. It prevents:

SELECT name, name
FROM Author

and

SELECT name, *
FROM Author

Okie-doke, then 👍

…cord into RelationMagicFunctionsAndSelectFix # Conflicts: # lib/Model.php # lib/Relation.php

- Implemented testReselect

…Test.php and ActiveRecordFindTest.php

- Fixed select() gets clobbered by count()

codecov · 2023-09-16T00:08:44Z

Codecov Report

Merging #69 (71e4187) into master (99c5f67) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master      #69      +/-   ##
============================================
+ Coverage     96.17%   96.26%   +0.09%     
- Complexity      936      962      +26     
============================================
  Files            33       33              
  Lines          2274     2330      +56     
============================================
+ Hits           2187     2243      +56     
  Misses           87       87

Files Changed	Coverage Δ
lib/Model.php	`98.03% <100.00%> (+0.03%)`	⬆️
lib/Relation.php	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

shmax

Looking pretty good. 👍

shmax · 2023-09-15T23:57:10Z

lib/Model.php

@@ -1751,7 +1774,14 @@ protected static function Relation(array $options = []): Relation
     */
    public static function count(/* ... */): int
    {
-        return static::Relation()->count(...func_get_args());
+        $relation = static::Relation();


If you're going to do any work, please delegate to Relation, like we do everywhere else.

And didn't we remove support for passing conditions to count? I don't see any mention of that in the modern rails documentation:
https://apidock.com/rails/v5.2.3/ActiveRecord/Calculations/count

What prompted this change?

Backwards compatibility for version 1. What I had in my original PR was Model::find() would have the complex support for version 1 parameters, and Relation::find() would only support what's in modern Ruby. If you want to break every single user of version 1 (ie: all of us) in version 2, then this is no longer necessary. The users of version 2 would do Model::where(complex input stuff)->count()

if someone wants to start a 1.x legacy branch and start back-porting fixes then that's up to them, but otherwise I'm not interested in bc; this API is confusing enough without muddying things further. Once we have our shiny new 2.x line, we will be more careful going forward.

Oh, so that's why you removed all those version 1 tests and threw away the code I wrote in my original PR to be version 1 compatible. If that's not a design goal, then that simplifies implementation, but will probably upset the entire version 1 user base as everybody will need to change their code. But that's the meaning of going from version 1 to 2. It was possible to keep backwards compatibility like I showed for count(). Anyway, removed backwards compatibility.

Yeah, sorry about that, but even if we did keep passing where clauses to find and count it still wouldn't make v2.0 compatible with 1.x (because, for example, I moved us away from mixed-array association configurations in the interest of static checking). I guess what we could do is find the earliest point in the commit history where all the tests pass and tag it with 1.2, or something, but we would have to revert the config changes, and I just don't have the energy. Feel free to volunteer, though.

We could look at cc36507 as a potential 1.2 release. I'll try it out in my own codebase at some point and see what happens.

lib/Model.php

shmax · 2023-09-16T00:02:25Z

lib/Relation.php

-     *
-     * @param string $propertyName Supports only first|last|take
+     * Returns a blank Relation and fires no queries. Any subsequent call to find(), to_a(), last(), first() returns []
+     */


Return type?

It's there, but put in doc block too now

I think you want one of these:

* @returns Relation<TModel>

lib/Relation.php

shmax · 2023-09-16T00:57:14Z

lib/Relation.php

-        $this->options['select'] = array_merge((array) $this->options['select'], (array) $arg);
+        $columns = array_merge((array) $this->options['select'], $columns);
+
+        return $this->reselect($columns);


It seems a bit overly busy to keep crunching the values down on the fly. My instinct would be to just collect information as-is, and do any processing at the end.

FWIW, that seems to be how Rails does it, as well:

I'm not sure what you mean here. The performance hit one way or another for manipulating short arrays in memory pales in comparison to the actual database hit. So, if performance is concern here, this is not likely going to make a visible difference when using this library. Turning SQL statements would make a much more important improvement in speed.

I'm not saying you can't collapse the SELECT statements, I'm saying you should do it in the same place the rest of this kind of work happens, in Table::options_to_sql. Then it's all in one place, and people working with the debugger will be less likely to fall victim to the principle of least surprise. And no, it probably won't amount to much of a performance savings, but that's not exactly a reason not to do it.

If you're still not convinced we don't have to let it hold up this PR, but be warned that I will likely move things around soon after.

I'm gone for the weekend, so please feel free to do that. I'm glad we could get this merged before I leave.

Fixed in #77

lib/Relation.php

test/ActiveRecordSelectTest.php

shmax · 2023-09-16T01:09:19Z

test/ActiveRecordSelectTest.php

+        $this->expectException(UndefinedPropertyException::class);
+        $author = Author::select('name')->reselect('author_id')->first();
+        $author->name;
+        $this->fail('expected ActiveRecord\UndefinedPropertyExecption');


I don't think you need this line; as a matter of fact, I think they can likely all be removed.

I've copied and pasted $this->fail('expected ActiveRecord\UndefinedPropertyExecption'); from other tests, but didn't know what they were there for. Removed.

Yeah, that's my fault, I added all those expectException lines, but didn't catch all the fail calls. Thanks much.

Thanks for clarifying.

I tracked the other extraneous $this->fail() functions and removed them in the latest commit

Thanks much.

…tyExecption'); lines

lib/Relation.php

shmax · 2023-09-16T03:49:25Z

Awesome, thanks!

ipundit added 30 commits August 31, 2023 18:10

Primary key has to be lowercased too because gettter/setters are lowe…

e1bb78f

…rcased in Model.php in order to be case insensitive.

Primary key has to be lowercased too because gettter/setters are lowe…

67471f5

…rcased in Model.php in order to be case insensitive.

Merge branch 'master' of https://github.com/ipundit/activerecord

83a4db0

# Conflicts: # lib/Model.php

Merge branch 'master' into master

1aea4e1

Merge branch 'master' of https://github.com/php-activerecord/activere…

9639f31

…cord # Conflicts: # test/ActiveRecordTest.php # test/models/Book.php

Running all the tests may take more than the default of 300 seconds. …

52478ab

…Disable it so that regression does not timeout.

Write proper tests that demonstrate the fix for issue php-activerecor…

c323677

…d#35: Authors table now has a column that is camel cased: firstName Added tests that demonstrate case insensitivity when accessing this firstName column

Switched from firstName field to existing mixedCaseField to pass Post…

80ed052

…gres regression

- List things in alphabetical order

b8ecfc0

- Correct link to contributing.md

Revert updates to test suite as ActiveRecordTest::test_case_insensiti…

23d00a7

…vity() is enough.

Merge branch 'master' of https://github.com/php-activerecord/activere…

e31ec4f

…cord

Merge branch 'master' of https://github.com/php-activerecord/activere…

9a6fb32

…cord

Merge branch 'master' of https://github.com/php-activerecord/activere…

ba7f090

…cord

Implemented composer test <fileName> <filter>

d42a7d1

Merge branch 'master' of https://github.com/php-activerecord/activere…

fc79d97

…cord

Merge branch 'master' of https://github.com/php-activerecord/activere…

568cfb5

…cord # Conflicts: # composer.json # scripts/TestCommand.php

Fix style issues

650d368

Merge branch 'master' of https://github.com/php-activerecord/activere…

767e537

…cord

Merge branch 'master' of https://github.com/php-activerecord/activere…

089d3d3

…cord

Merge branch 'master' of https://github.com/php-activerecord/activere…

2eec6b0

…cord

Fix for issue php-activerecord#55

6aba443

Remove unused scripts and files references.

c772578

Merge branch 'master' of https://github.com/php-activerecord/activere…

8291ea6

…cord

Merge branch 'master' of https://github.com/php-activerecord/activere…

cf6056e

…cord into FixWarning

Remove unused items in composer.json

64e45b8

Merge branch 'master' of https://github.com/php-activerecord/activere…

1dcad8d

…cord

Merge branch 'master' of https://github.com/ipundit/activerecord

7b1e20e

Merge branch 'master' of https://github.com/php-activerecord/activere…

79d2182

…cord

ipundit added 3 commits September 14, 2023 00:53

Document composer style-fix

9a37195

Merge branch 'master' of https://github.com/php-activerecord/activere…

929aa16

…cord into RelationMagicFunctionsAndSelectFix

Remove extraneous whitespace when converting comma separated columns

eab774d

shmax requested changes Sep 14, 2023

View reviewed changes

ipundit added 6 commits September 15, 2023 11:46

Merge branch 'master' of https://github.com/php-activerecord/activere…

69e9009

…cord into RelationMagicFunctionsAndSelectFix # Conflicts: # lib/Model.php # lib/Relation.php

- Moved select tests from RelationTest.php to ActiveRecordSelectTest.php

11bbdd4

- Implemented testReselect

Moved duplicated tests from RelationTest.php to ActiveRecordFirstLast…

e7f24ae

…Test.php and ActiveRecordFindTest.php

- Implemented reselect(), none(), pluck(), ids()

d897cfa

- Fixed select() gets clobbered by count()

Fixed test\ActiveRecordGroupTest::testHaving()

57a2fcb

ran composer style-fix

6ee97d7

Improve code coverage

1c71040

ipundit requested a review from shmax September 16, 2023 00:46

ipundit mentioned this pull request Sep 16, 2023

Mixed-case column names causing trouble on postgresql #64

Closed

ipundit added 2 commits September 15, 2023 20:51

Fixed backwards compatibility test

35673b4

Fixed stan test

c1f4c79

ipundit changed the title ~~Relation magic functions and select fix~~ Fixed select(), implemented pluck(), ids(), reselect(), none() Sep 16, 2023

shmax reviewed Sep 16, 2023

View reviewed changes

ipundit added 4 commits September 15, 2023 21:39

In response to code review comments

121453b

Style-fix

af61f14

Removed extraneous $this->fail('expected ActiveRecord\UndefinedProper…

096f494

…tyExecption'); lines

Fixed a copy and paste error for the return type.

b78e8b6

shmax reviewed Sep 16, 2023

View reviewed changes

lib/Relation.php Show resolved Hide resolved

ipundit added 3 commits September 15, 2023 21:50

Removed backwards compatibility for count()

ee05bc9

Fixed return type

a10f50b

Simplify

71e4187

shmax approved these changes Sep 16, 2023

View reviewed changes

shmax merged commit 830ecc9 into php-activerecord:master Sep 16, 2023

ipundit deleted the RelationMagicFunctionsAndSelectFix branch September 18, 2023 10:36

Fixed select(), implemented pluck(), ids(), reselect(), none() #69

Fixed select(), implemented pluck(), ids(), reselect(), none() #69

Uh oh!

Conversation

ipundit commented Sep 14, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shmax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shmax Sep 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shmax commented Sep 16, 2023

Uh oh!

Uh oh!

codecov bot commented Sep 16, 2023 •

edited

Loading

shmax Sep 16, 2023 •

edited

Loading