Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore Query::one interface #798

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Restore Query::one interface #798

wants to merge 5 commits into from

Conversation

darkdef
Copy link
Contributor

@darkdef darkdef commented Jan 21, 2024

Q A
Is bugfix? ✔️
New feature? ✔️
Breaks BC? ✔️/❌

Copy link

codecov bot commented Jan 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.55%. Comparing base (749c3f3) to head (6e904c6).

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #798   +/-   ##
=========================================
  Coverage     99.55%   99.55%           
  Complexity     1277     1277           
=========================================
  Files            63       63           
  Lines          3120     3120           
=========================================
  Hits           3106     3106           
  Misses           14       14           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@darkdef darkdef added this to the 2.0.0 milestone Jan 21, 2024
Copy link
Member

@Tigrov Tigrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BatchQueryResult::$value and related methods also should have the type array|object|null

private mixed $value;

Copy link

what-the-diff bot commented Jan 29, 2024

PR Summary

  • Modifications to the queryOne() method
    The queryOne() function, which can be found in AbstractCommand.php, CommandInterface.php, and CommandInterfaceProxy.php, underwent revisions that enable it to provide a more diverse set of results. This function formerly returned only arrays, but with this update, it can now return an array, an object, or even a null value. This improvement enhances the flexibility and coverage of the response types our system can deliver.

  • Improvements to the one() method
    Similar to the queryOne() modifications, we have updated the one() function in Query.php and QueryInterface.php to return either an array, an object, or a null value. Previously, the function was limited to just array results. This change broadens the scope of possible outputs, making our system more diverse and adaptable in responding to different query requirements.

@darkdef
Copy link
Contributor Author

darkdef commented Jan 29, 2024

BatchQueryResult::$value and related methods also should have the type array|object|null

It's maybe incorrect for query with one column as scalar type.

@Tigrov
Copy link
Member

Tigrov commented Jan 30, 2024

BatchQueryResult::$value and related methods also should have the type array|object|null

It's maybe incorrect for query with one column as scalar type.

This should be an array or object ($row) or array of arrays or objects ($rows) or null, but seems this is not related with the PR.

$this->dataReader = $this->query->createCommand()->query();

public function query(): DataReaderInterface
{
/** @psalm-var DataReaderInterface */
return $this->queryInternal(self::QUERY_MODE_CURSOR);
}

And after custom populate closure $rows can be changed to array of objects

if ($this->populateMethod !== null) {
return (array) ($this->populateMethod)($rows, $this->query->getIndexBy());
}

Copy link
Member

@Tigrov Tigrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs line to changelog

Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need also fix type of CommandInterface::queryAll()?

Comment on lines +53 to +54
* @return array The query results. If the query results in nothing, it returns an empty array.
* @psalm-return array[]|object[]
Copy link
Member

@vjik vjik Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return array The query results. If the query results in nothing, it returns an empty array.
* @psalm-return array[]|object[]
* @return array The query results. If the query results in nothing, it returns an empty array.
* @psalm-return array<array-key,array|object>

@@ -631,10 +631,10 @@ public function queryColumn(): array;
* @throws Exception
* @throws Throwable If execution failed.
*
* @return array|null The first row (in terms of an array) of the query result. Null if the query
* @return array|object|null The first row (in terms of an array) of the query result. Null if the query
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return array|object|null The first row (in terms of an array) of the query result. Null if the query
* @return array|object|null The first row (as array or object) of the query result. Null if the query

@vjik
Copy link
Member

vjik commented Apr 7, 2024

Needs line to changelog

And fill UPGRADE.md too

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.

None yet

5 participants