-
Notifications
You must be signed in to change notification settings - Fork 2k
refactor: Update types for BaseModel, Model and dependencies
#9711
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
Conversation
42136ab to
11a3252
Compare
michalsn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Some methods have a
$testingparameter, which set totrue, will not execute the query and just return the SQL. -
No array - as we do not support composite keys in the model right now. Support for
0and'0'is problematic, and I would rather not allow it, as it will cause problems when combined with$db->insertID(), which uses0to indicate a failure.
|
michalsn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The message can stay, for reference.
|
👋 Hi, @neznaika0! |
e985209 to
0174725
Compare
|
Is there a solution for PHPDoc types? Take a look if you have time https://talk.typo3.org/t/phpstan-specific-phpdoc-annotations-in-the-core-code-base/5504/1 |
|
Unfortunately, we barely discuss common questions. There are no exact definitions of how to write code. I'm closing the PR. |
Description
Updated PHPDoc, clarified arrays, duplicate comments have been removed from the Model and did some work with dots.
I generated the API using phpDocumentor. I didn't notice any serious problems.
Some questions:
$idforBaseModel::doFind()doFindColumn() findColumn()- not sure about the type accuracyBaseModel::getIdValue()does not return an array. Right? I have removed PHPDoc and array checking. Is it possible to use 0, '0' for ID? Otherwise, I would have added the checks toBaseModel::shouldUpdate().Checklist: