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.

* Used by asArray() and asObject() to provide
* The temporary format of the result.
*
* Used by `$this->asArray()` and `$this->asObject()` to provide
Copy link
Member

Choose a reason for hiding this comment

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

Why are you adding $this everywhere? It makes the comments longer and doesn't add value (at least from my perspective). Do backticks actually change the syntax in phpDocumentor? If yes, please use them consistently everywhere - if not, please remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, phpDocumentor supports Markdown. Therefore, we can mark some code as bold... I added a clarification to explicitly indicate a method rather than a function. This may seem redundant, but it's not a problem to reduce it. We're considering a PR together to address this.

@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.

*
* @throws InvalidArgumentException
*/
protected function hasRationalPrimaryKey($id): bool
Copy link
Member

Choose a reason for hiding this comment

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

The primary key in our model refers more to the column name, so this may be misleading.

I have no idea how we can name it... isUsableValueId()? 🤷‍♂️

The goal for these checks is purely to satisfy PHPStan or validate the allowed data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I remove phpstan errors. The next step was formed by itself - we can validate the ID value.

I would set the relationship to PK. For now, all checks are used for it. But it can be renamed to hasRationalId() if it is used for table joins, for example.

Copy link
Member

Choose a reason for hiding this comment

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

The "rational" part of the method doesn't really speak to me, to be honest.

Well... let's forget about validating the primary key for now - I'm not convinced we need to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove the check or what, back to if ($id)?

rational - based on logic. If there is a problem with the translation.

Copy link
Member

Choose a reason for hiding this comment

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

Using if ($id) would likely bring us back to square one, with PHPStan raising an error.

Yes, I know what “rational” means, but I don't think it's a good fit here.

I'm leaning towards leaving the code as it is (without introducing an additional method), but unifying the values in in_array. Happy to hear others' opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samsonasik @paulbalandan i need help.

@github-actions github-actions bot added the stale Pull requests with conflicts label Sep 10, 2025
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

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.

2 participants