Skip to content

Conversation

neznaika0
Copy link
Contributor

Description
Improved typing.
What is done:

  • Added a "dot" at the end of the descriptions
  • Since CIUnitTestCase, the dependent code has been updated
  • Updated types in related functions in Common.php
  • Properties have been added for traits. If it is redundant, some can be deleted, phpstan does not consider this a mistake.

OFFTOP: I'll ask again: Do we really need repeated code descriptions? (You answered Yes.)
Because of this, it needs to be fixed in several places. See, example $namespace property. Modern IDEs fully support type highlighting and you can switch to declarations. If not, then is it worth adding missing comments everywhere? I would leave it as it is now, but this contradicts the desire to duplicate the code: Everywhere without repeating, or always duplicate typing.

Checklist:

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

@paulbalandan paulbalandan added the refactor Pull requests that refactor code label Aug 15, 2025
@paulbalandan
Copy link
Member

I'm not a fan either of duplicating code comments and PHPDocs. If those are already defined by the base class or interface, then child classes can remove them since, as you said, IDEs will show you the inherited code comments (unless you are not using an IDE?). It is also a maintenance burden to keep similar comments in sync.

@neznaika0
Copy link
Contributor Author

If I'm not mistaken, @samsonasik is against deletion.

@samsonasik
Copy link
Member

can @inheritDoc be used?

@neznaika0
Copy link
Contributor Author

Why? Inheritance works fine without it. Maybe the tag is needed when generating documentation? I don't see the need for it during development.

Look at $namespace, $seed - the properties have different values, but the type is specified again.

@paulbalandan
Copy link
Member

If I remember correctly, @inheritDoc is needed by phpDocumentor for its generated API docs. But IDEs work perfectly without it and there are other documentation generators that can generate API docs without @inheritDoc.

@neznaika0
Copy link
Contributor Author

Tags should be updated if the type changes or becomes more precise (int|string > object). Am I wrong?

@paulbalandan
Copy link
Member

Yes, they should be updated.

@michalsn
Copy link
Member

If phpDocumentor requires @inheritDoc, then we should include it.

@neznaika0
Copy link
Contributor Author

Okay. Can we fix the style in the Markdown guidelines or user guide?

I would like to have an exact strategy for further refactoring (example Commands, Request, Model,..). It may be necessary to remove all "bad" files from the comments. Alternatively, you can add @inheritDoc.

This may reduce the size of the installation ZIP archive.

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

@paulbalandan
Copy link
Member

I guess the strategy would be:

  1. If an interface or base class defines a PHPDoc, child classes DO NOT need to replicate that. They can put @inheritDoc in place.
  2. If a child class provides a narrower return type PHPDoc than the parent class, it can still use inheritDoc but must include the narrower return.

That's what I can think of for now. And I don't know what bad files you mean.

@neznaika0
Copy link
Contributor Author

Bad files are those that already duplicate comments.

@neznaika0 neznaika0 force-pushed the refactor/phpdoc-improve branch from ca2ae9e to ec47048 Compare September 2, 2025 04:18
@ddevsr ddevsr removed the stale Pull requests with conflicts label Sep 2, 2025
@neznaika0
Copy link
Contributor Author

I tested phpDocumentor.

  1. Changes to a child property overwrites the parent value.
  2. The empty/{@inheritDoc}/undefined value of PHPDoc uses the parent value. That's what we need.
  3. To add PHPDoc, you can copy the comment from the parent. See https://docs.phpdoc.org/guide/guides/inheritance.html#inheritance - {@inheritDoc} copies the parent comment for addition. Optional by default.

It follows that there is no 100% need for duplicate comments.

Will it be possible to clean it up in future PR? Sorry for my perfectionism). Results:

  1. Reduces code refactoring.
  2. Reduces the size of the total code (for ZIP installation)
  3. Reduces scrolling of long files.

@michalsn
Copy link
Member

michalsn commented Sep 4, 2025

I know this was approved, but since then, we have had some additional discussion and commits here. Is this good to merge?

@paulbalandan
Copy link
Member

Yes, this is good to merge.

@neznaika0
Copy link
Contributor Author

I would clean up the duplicates before merging, if you agree.

@michalsn
Copy link
Member

michalsn commented Sep 5, 2025

Are these duplicates related to phpstan?

@neznaika0
Copy link
Contributor Author

Yes. Is this PHPDoc for child properties: namespace, seed, seeder,... I would update all the tests where they use CIUnitTestCase or traits, leaving only the definition.
For example, only protected $namespace = 'Tests\Support'; without comments /** */.

@michalsn
Copy link
Member

michalsn commented Sep 5, 2025

Okay, it's fine by me.

@neznaika0 neznaika0 force-pushed the refactor/phpdoc-improve branch from ec47048 to be462ec Compare September 5, 2025 14:24
@michalsn michalsn merged commit aa2dcd6 into codeigniter4:develop Sep 5, 2025
49 checks passed
@michalsn
Copy link
Member

michalsn commented Sep 5, 2025

Thank you @neznaika0

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

Labels

refactor Pull requests that refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants