Skip to content

Conversation

@neznaika0
Copy link
Contributor

@neznaika0 neznaika0 commented Sep 6, 2025

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:

  1. I did not find a test for sampling from the array $id for BaseModel::doFind()
  2. Can someone explain the phrase? Can it be improved?

    This method works only with dbCalls

  3. phpDocumentor does not understand conditional values for @return, it is necessary to duplicate complex conditions in @phpstan-return
  4. Look at doFindColumn() findColumn() - not sure about the type accuracy
  5. ⚠️ BaseModel::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 to BaseModel::shouldUpdate().
  6. Tell me if you need to update the guide or changelog.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@neznaika0 neznaika0 force-pushed the refactor/model-types branch from 42136ab to 11a3252 Compare September 6, 2025 16:29
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

  1. Some methods have a $testing parameter, which set to true, will not execute the query and just return the SQL.

  2. No array - as we do not support composite keys in the model right now. Support for 0 and '0' is problematic, and I would rather not allow it, as it will cause problems when combined with $db->insertID(), which uses 0 to indicate a failure.

@neznaika0
Copy link
Contributor Author

  1. Some methods have a $testing parameter, which set to true, will not execute the query and just return the SQL.
  2. No array - as we do not support composite keys in the model right now. Support for 0 and '0' is problematic, and I would rather not allow it, as it will cause problems when combined with $db->insertID(), which uses 0 to indicate a failure.
  1. I don't need to change the message?
  2. not 3? I'll add a check 0? You have an issue Possible bug: Model functions (doDelete, doFirst, etc..) ignores zero as id #9577 where it's found: that in general, the $id check is different in methods. Is it acceptable to move it to a separate method if possible?

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

  1. The message can stay, for reference.

@github-actions github-actions bot added the stale Pull requests with conflicts label Sep 10, 2025
@github-actions
Copy link

👋 Hi, @neznaika0!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@neznaika0
Copy link
Contributor Author

neznaika0 commented Oct 12, 2025

Is there a solution for PHPDoc types?
I've been doing some reading here - using array<> list<> non-empty-string array-key is also not a default for PHPDoc. Preferred Type Type[]

Take a look if you have time https://talk.typo3.org/t/phpstan-specific-phpdoc-annotations-in-the-core-code-base/5504/1

@neznaika0
Copy link
Contributor Author

Unfortunately, we barely discuss common questions. There are no exact definitions of how to write code. I'm closing the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Pull requests with conflicts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants