Fixed quoted pgsql column names #87
Conversation
…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
- Correct link to contributing.md
…vity() is enough.
…cord # Conflicts: # composer.json # scripts/TestCommand.php
|
Implemented not(Relation) in #83 |
|
It's great to see so much energy, but in the interest of moving this forward, please don't piggy-back unrelated features. |
|
This passes |
|
You seem to have some failed tests: |
|
I cannot reproduce these failures D:\activerecord>git pull
Already up to date.
D:\activerecord>composer test Relation
> Composer\Config::disableProcessTimeout
> ActiveRecord\Scripts\TestCommand::runTest
Running: vendor/bin/phpunit test/RelationTest.php
To run a specific test in test/DateTimeTest.php, try: composer test dateTime testSetIsoDate
(Cache Tests will be skipped, Memcache not found.)
PHPUnit 10.3.2 by Sebastian Bergmann and contributors.
Runtime: PHP 8.2.9
Configuration: D:\activerecord\phpunit.xml.dist
.................. 18 / 18 (100%)
Time: 00:10.447, Memory: 10.00 MB
OK (18 tests, 61 assertions) |
|
Make sure you run the whole test suite on local. When a single class passes on its own but fails in the context of the entire suite it's generally a sign that there is cross pollution of state happening between tests (eg. static members on models and whatnot). |
| * @param string $expression The where clause to be escaped | ||
| * @param array<string> $columns The columns of the table | ||
| */ | ||
| public function escapeColumns(string $expression, array $columns): string |
There was a problem hiding this comment.
Better, but still a bit complex, isn't it? I would think you could do it all in one method, something like:
public function escapeColumns(string $expression, array $columns): string
{
$cachedNames = [];
foreach ($columns as $column) {
if ($column !== strtolower($column)) {
$cachedNames[$column] ??= $this->quote_name($column);
$expression = str_replace($column, $cachedNames($column), $expression);
}
}
return $expression;
}I didn't test it (I think I lost an implode in there, somewhere), and I'm sure you can think of something better, but maybe it'll give you a few ideas.
There was a problem hiding this comment.
New implementation is a bit shorter and more performant. escapeColumns() is called several times over with the exact same $columns passed in. Converting that $columns into a lookup hash prevents having to iterate over and over it again.
There was a problem hiding this comment.
Then you can use a static:
static $cachedNames = [];
foreach ($columns as $column) {
if ($column !== strtolower($column)) {
$cachedNames[$column] ??= $this->quote_name($column);
$expression = str_replace($column, $cachedNames($column), $expression);
}
}
return $expression;
There was a problem hiding this comment.
This is not as performant as the original implementation. The foreach ($columns as $column) { will be run over and over again for all the columns of a table. Most of them if you're following Postgres best practices will be lower cased.
There was a problem hiding this comment.
Yeah, I'll take care of it, thanks.
Simplified escapeColumns()
|
Hi, can you please move the and/or/not stuff to dedicated PRs? It's too hard to review what should have been a fairly simple change when you keep adding stuff that's not related. Thank you! |
|
I've had enough. I've asked you twice now to stop tacking on code for unrelated features. Until you get the message, I'm closing this PR, and not entertaining new ones. |
|
Sorry, I'm rushing to push this code before a deadline. I am not able to work anymore this week due to work commitments, and this is the last code push I can do this week, without time to split into several PR's. If you don't need my help anymore on this project and won't accept any more PR's, then at least this last one gives you |
|
Your contributions are welcome, but let's establish some guidelines:
|
Fixed #64
This bug was not actually in my code, and a reversion of PR #36 would not have fixed this issue. Instead, the use of camelCase in pgsql must be quoted everywhere it's used, so the test cases had to be updated in Relation. Regression passes in pgsql and mysql, and the mixedCaseField is quoted and not quoted respectively for these test cases.