Skip to content
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

Various phpdoc fixes #22509

Merged
merged 1 commit into from
Jan 14, 2022
Merged

Various phpdoc fixes #22509

merged 1 commit into from
Jan 14, 2022

Conversation

braders
Copy link
Contributor

@braders braders commented Jan 14, 2022

Overview

Lots more phpdoc fixes.

Largely these handle the case where previously the type definition was incorrectly placed at the end of the @param line, rather than before the variable name.

Before

PHPdoc inaccuracies.

After

PHPdoc (more) accurare.

Comments

This is far from comprehensive - there are still lots of PHPdoc issues left after this set of changes.

@civibot
Copy link

civibot bot commented Jan 14, 2022

(Standard links)

@civibot civibot bot added the master label Jan 14, 2022
@@ -378,7 +389,6 @@ public static function fixInputParams(&$input) {
}
}
}
return NULL;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return type was not being used anywhere, and the docblock was previously being contradictory. void ultimately felt more appropriate.

@colemanw
Copy link
Member

colemanw commented Jan 14, 2022

Great cleanup, excellent work.
Jenkins is a bit grumpy about one of the files though. If you click the "Details" link ^ and then click a few more times to drill down you get to this:
image

@@ -319,7 +320,7 @@ public static function getContactTags($contactID, $count = FALSE) {

if ($count) {
$dao->fetch();
return $dao->cnt;
return (int) $dao->cnt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Because cnt is sooo much shorter than count .....

Copy link
Member

Choose a reason for hiding this comment

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

Cuter too

Copy link
Member

Choose a reason for hiding this comment

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

I'm fairly certain that I've seen errors in the past when trying to use count as a column-alias in DAO queries. Never traced the cause - just made a mental note to go along with cnt as the typical work-around name.

But... in the current stack, it looks like SELECT ... AS count works fine. So I'm +1 for the count...

Copy link
Contributor

Choose a reason for hiding this comment

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

@totten you do know my comments about always 'reading' variables applies - there is only one way to read cnt outloud

@totten totten merged commit ed5ce36 into civicrm:master Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants