Skip to content

Improve PHPDoc Types#223

Open
james-cnz wants to merge 1 commit intomoodlehq:mainfrom
james-cnz:improve_phpdoc_types
Open

Improve PHPDoc Types#223
james-cnz wants to merge 1 commit intomoodlehq:mainfrom
james-cnz:improve_phpdoc_types

Conversation

@james-cnz
Copy link
Copy Markdown

I've run a basic PHPDoc type checker over the code, and tidied up a few things. I think it's probably good for moodle-cs to set a good example.

Note: I know the PHPCS Sniff function process() specifies its return type as void|int, but AFAIK, void is a stand-alone type, and int|null is the correct type.

@jrchamp
Copy link
Copy Markdown
Contributor

jrchamp commented Dec 28, 2025

None of the process() methods that I looked at had a return that was anything other than void (i.e. return;). Where are you seeing int returns? Also, if nullable int were the correct return value, I'd lean towards ?int over int|null because - to me - it implies "sometimes you get an integer" as opposed to "sometimes you get an integer which means A and sometimes you get null which means B".

I hope that helps! I'm sure others will have more comprehensive feedback.

@james-cnz
Copy link
Copy Markdown
Author

Yeah, I don't think any of the process methods in moodle-cs return int. It would also be valid to specify the return type as just null. It's in the base method, though, and I didn't think there was any point narrowing it.
https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Sniffs/Sniff.php
The return statements should probably say return null; for good style, but PHP defaults to null if trying to read the return value of a function when none was provided.
I used int|null because it conforms to PSR-5, which I've been told the Moodle code base uses, so I've leaned into this style where possible. Maybe it's not appropriate for moodle-cs, though.

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.

2 participants