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

dev/core#2079 [Ref] Clarify calls to CRM_Utils_Token::getTokenDetails #18653

Merged
merged 1 commit into from
Oct 7, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 1, 2020

Overview

This changes the call to getTokenDetails to make it clearer that only the first parameter is used.

The second parameter is an unused query that we should eliminate but getting it
easier to scan is a good first step

Before

$tokenResolvedContacts = CRM_Utils_Token::getTokenDetails(['contact_id' => $contactID],

then used as $tokenResolvedContacts[0]

After

[$tokenResolvedContacts] = CRM_Utils_Token::getTokenDetails(['contact_id' => $contactID],

then used without the [0]

Technical Details

Comments

This changes the call to getTokenDetails to make it clearer that only the first parameter is used.

The second parameter is an unused query that we should eliminate but getting it
easier to scan is a good first step
@civibot
Copy link

civibot bot commented Oct 1, 2020

(Standard links)

@civibot civibot bot added the master label Oct 1, 2020
@eileenmcnaughton eileenmcnaughton changed the title [Ref] Clarify calls to CRM_Utils_Token::getTokenDetails dev/core#2079 [Ref] Clarify calls to CRM_Utils_Token::getTokenDetails Oct 1, 2020
@mattwire
Copy link
Contributor

mattwire commented Oct 1, 2020

@eileenmcnaughton Can you please update the comment for the return value on getTokenDetails() dockblock as well? Once I worked out why this PR makes sense and is correct but I'd have got there quicker if the docblock explained what params are in that return array.

@eileenmcnaughton
Copy link
Contributor Author

@mattwire I never really worked it out :-) My goal is to confirm the second value not used anywhere & then just remove it

@eileenmcnaughton
Copy link
Contributor Author

@mattwire the PR to remove it altogether is here #18657

@colemanw colemanw merged commit c626123 into civicrm:master Oct 7, 2020
@eileenmcnaughton eileenmcnaughton deleted the token branch October 7, 2020 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants