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

Clarify types on hook_custom and hook_customPre #20488

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 3, 2021

Overview

Clarify types on custom hooks

The custom & customPre hooks are not called elsewhere in gituniverse
so we can add type hints and fix casting & comments to make it
clearer what the variables are

Before

Types unclear & some incorrectly documented

After

Casting, hints, correct docs

Technical Details

Comments

The custom & customPre hooks are not called elsewhere in gituniverse
so we can add type hints and fix casting & comments to make it
clearer what the variables are
@civibot
Copy link

civibot bot commented Jun 3, 2021

(Standard links)

@civibot civibot bot added the master label Jun 3, 2021
@@ -48,8 +48,7 @@ public static function create($customParams, $parentOperation = NULL) {
$count = 1;

$firstField = reset($fields);
$entityID = $firstField['entity_id'];
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 brevity of casting to a var vs the loss of clarify is a little greater for entity_id as entityID is less vague & it's used more

@totten totten changed the title Clarify types on custom hooks Clarify types on hook_custom and hook_customPre Jun 3, 2021
@totten
Copy link
Member

totten commented Jun 3, 2021

Looks reasonable to me... I can't imagine a good reason why those parameters would have other types.

@totten totten added the merge ready PR will be merged after a few days if there are no objections label Jun 3, 2021
@eileenmcnaughton
Copy link
Contributor Author

@totten totten merged commit a9fc84c into civicrm:master Jun 4, 2021
@eileenmcnaughton eileenmcnaughton deleted the cust_strict branch June 4, 2021 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants