Skip to content

Conversation

@storyn26383
Copy link
Contributor

@storyn26383 storyn26383 commented Aug 27, 2025

原先 database component 中,PHPDoc 所標示的型別不少有錯誤或是不完整。
這個 PR 針對其做了 PHPDoc 的修正,並且補上了測試,讓型別標示更正確及完整。

@limingxinleo
Copy link
Member

types 这个目录也是单测?都放到 tests 目录里,你这个目录下我看都是 database 的单测,那就放到 database/tests 目录下面

@storyn26383
Copy link
Contributor Author

types 这个目录也是单测?都放到 tests 目录里,你这个目录下我看都是 database 的单测,那就放到 database/tests 目录下面

已調整~

@albertcht
Copy link
Contributor

@limingxinleo 再麻煩有空時 review 一下了

Copy link
Member

@huangdijia huangdijia left a comment

Choose a reason for hiding this comment

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

Collection是不是也要跟随调整?


/**
* @template TValue
* @template TValue of object
Copy link
Member

Choose a reason for hiding this comment

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

of object or of Model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be object, because this trait used by not only Model\Builder but Query\Builder

@huangdijia huangdijia requested a review from Copilot September 24, 2025 22:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to improve the type annotations in the Hyperf database component by adding comprehensive PHPDoc type hints and creating extensive type tests. The changes enhance static analysis capabilities and provide better IDE support for developers working with the database ORM.

  • Improved PHPDoc type annotations throughout database component classes for better static analysis
  • Added comprehensive type test files to verify type correctness and coverage
  • Enhanced generic type support for Model relationships, Collections, and Builders

Reviewed Changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/scout/src/SearchableScope.php Added PHPStan ignore comment for type issue
src/database/tests/types/ Added comprehensive type test files for Query Builder, Model relations, Collections, and other components
src/database/src/ Updated PHPDoc annotations with generic types and improved return type specifications
phpstan.types.neon.dist Added PHPStan configuration for type testing
composer.json Added new script for running type analysis
CHANGELOG-3.1.md Documented the PHPDoc optimization
.travis/run.check.sh Added type analysis to CI checks

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

*
* @param array|BaseCollection|Model $ids
* @return array
* @return array{attached: array, detached: array, updated: array}
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

The array shape documentation could be more specific. Consider using array<mixed, mixed> or defining the key-value types more precisely for the attached, detached, and updated arrays to provide better type safety.

Copilot uses AI. Check for mistakes.
Comment on lines +489 to +491
if (is_string($relation)) {
$relation = $this->getRelationWithoutConstraints($relation);
}
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

[nitpick] This guard clause should be extracted into a helper method since it's duplicated logic that appears to convert string relation names to relation instances. Consider creating a resolveRelation method to reduce code duplication.

Copilot uses AI. Check for mistakes.
Comment on lines 40 to +43
* @param mixed $key
* @param TFindDefault $default
* @return static<TKey|TModel>|TFindDefault|TModel
* @return ($key is array ? static : TFindDefault|TModel)
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

[nitpick] The conditional return type using ($key is array ? static : TFindDefault|TModel) is complex. Consider breaking this into separate @return annotations for different parameter types to improve readability and maintainability of the documentation.

Copilot uses AI. Check for mistakes.
* Chunk the results of a query by comparing numeric IDs.
*
* @param int $count
* @param callable(Collection<int, object>, int): mixed $callback
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

The callback parameter documentation uses generic object type. Since this is in the Query Builder context, consider using a more specific type like stdClass or documenting that the object represents a database row result.

Suggested change
* @param callable(Collection<int, object>, int): mixed $callback
* @param callable(Collection<int, \stdClass>, int): mixed $callback

Copilot uses AI. Check for mistakes.
@storyn26383
Copy link
Contributor Author

Collection 或許之後再調整比較好?這個 PR 先專注在 Database

@albertcht
Copy link
Contributor

@huangdijia , 再麻煩有空時 review 一下,感謝~

huangdijia
huangdijia previously approved these changes Oct 28, 2025
@limingxinleo
Copy link
Member

辛苦了,上次我以为我合并了。。

@limingxinleo limingxinleo changed the title 修正 database component 的型別標示 Optimized the doc comments for hyperf/database. Oct 28, 2025
limingxinleo
limingxinleo previously approved these changes Oct 28, 2025
@limingxinleo limingxinleo changed the title Optimized the doc comments for hyperf/database. Optimized the PHPDoc for hyperf/database. Oct 28, 2025
@limingxinleo limingxinleo merged commit 1b51ffa into hyperf:master Oct 28, 2025
@limingxinleo limingxinleo mentioned this pull request Oct 30, 2025
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.

4 participants