Convert query('SELECT ... FROM {xxx}') to select('xxx')->... in tests

It is interesting to check how count query affects performance. Probably query_alter hooks still fired if "select" query used

Some of this places using query() instead of select() to not allow alter the query with hook and save performance

Rerolled.

Looks good

There are 44 more occurrences in test cases?

Got it. Thanks for the explanation! I can confirm the remaining usages are indeed combined with db_query() and are suitable for tackling in those usages.

Let's do #3000016: Convert query('SELECT COUNT(*) FROM ...') to select('...')->countQuery() in tests (step 2) together with this splitting the issue doesn't really make sense.

Looks like this patch cover all count query in tests.
but needs reroll

+1 for RTBC

Tests still green

Hm...

> There's a better and more db abstract way to achieve the same with $injected_database->select(...)->countQuery()

Better is actually questionable. The problem with countQuery() is that it's inefficient. It basically does SELECT COUNT(*) from ($original_query); which can get very slow with certain queries, e.g. when using group by/distinct. That implementation was done to ensure that the results are always complex, even with very complex queries. That's also why the Pager extender has a method to set a custom, optimized count query.

When to use query() vs select() has been discussed since DBTNG has been added in Drupal 7.x. Some people disagree with that, but the original intent was that select() should only be used if necessary. For example for dynamic/alterable queries or when a query can not be written in a way that works on all backends. The select query builder is quite a bit slower than a simple string obviously. Again, the least of our problems in our tests, but wanted to point that out either way..

That doesn't matter much in tests, but that might not be true at all for real queries which I think was moved to a separate issue.

Second, looking at the converted examples, I see a lot of queries against aggregator_item, aggregator_feed. Those are entity tables, and query() and select() against a content entity table are both equally wrong, if we do bother to touch those, I'd better use entity queries.

EntityApiTest is a different, as that tests the actual implementation and specific tables, it makes sense there to check the specific tables.

I'd prefer to split the issue into
- convert entity related queries to entity ones, that could be a bug report
- discus the way we replace direct count queries which is fast with slow alterable "select" queries

this is failing on Postgres?

thanks

I agree with @andypost in #31 - where we're querying entity tables, we should just convert the whole way to an entity query. Would suggest a new issue for that, and leave the other conversions here.

Filed follow-up #3012599: Replace all db calls to aggregator_feed and aggregator_item tables with Entity APIs

Looks good. +1 for RTBC

I would have expected this patch to change db_query() to Connection::query()/$this->connection->query(), instead this is making static queries dynamic - we haven't deprecated static queries so not sure why it's doing that to be honest.

As you can see from the issue summary there, it's suggesting replacing db_query() with ::query(), not db_query() with ::select().

Quoting it here for quick reference:

# Replace the occurrences in code.
find ./core/lib -type f -exec sed -i -r 's/db_query\(/Database::getConnection\()->query\(/g;' {} \;
find ./core/tests -type f -not -path "./core/tests/Drupal/KernelTests/Core/Database/*" -exec sed -i -r 's/db_query\(/Database::getConnection\()->query\(/g;' {} \;
find ./core/tests/Drupal/KernelTests/Core/Database -type f -not -path "./core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php" -not -path "./core/tests/Drupal/KernelTests/Core/Database/QueryTest.php" -exec sed -i -r 's/db_query\(/$this->connection->query\(/g;' {} \;
find ./core/modules -type f -exec sed -i -r 's/db_query\(/Database::getConnection\()->query\(/g;' {} \;
find ./core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php -type f -exec sed -i -r "s/call_user_func_array\('db_query'/call_user_func_array\([\$this->connection, 'query']/g;" {} \;
find ./core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php -type f -exec sed -i -r 's/db_query\(/Database::getConnection\()->query\(/g;' {} \;
find ./core/includes/form.inc -type f -exec sed -i -r 's/db_query\(/Database::getConnection\()->query\(/g;' {} \;
find ./core/scripts/ -type f -exec sed -i -r 's/db_query\(/\\Drupal::database\()->query\(/g;' {} \;
# Start using \Drupal::database() or inject service
find ./core/modules/tracker -type f -exec sed -i -r 's/Database::getConnection\(/\\Drupal::database\(/g;' {} \;

Agree that this is not necessary. Per #48, I guess we can just close this as a won't fix then and convert it in #2875394: Replace all calls to db_query, which is deprecated?