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.

Sign up for GitHub

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

Jump to bottom

[REF] Extract code used to render a pseudoconstant when a table is defined. #16902

Merged
merged 1 commit into from
Mar 27, 2020
Merged

[REF] Extract code used to render a pseudoconstant when a table is defined. #16902

merged 1 commit into from
Mar 27, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 26, 2020 •
edited
Loading

Overview

Basic extraction in heavily tested code

This permits us to re-use for settings

Before

Code in one long function

After

Portion extracted out

Technical Details

Comments

@civibot
Copy link

civibot bot commented Mar 26, 2020

(Standard links)

@civibot civibot bot added the master label Mar 26, 2020
@eileenmcnaughton eileenmcnaughton changed the title [REF] Extract code used to render a pseudoconstant when a table is de… [REF] Extract code used to render a pseudoconstant when a table is defined. Mar 26, 2020
demeritcowboy
demeritcowboy reviewed Mar 26, 2020
View reviewed changes
CRM/Core/PseudoConstant.php
$output = CRM_Utils_Array::asort($output);
}
}
$output = self::renderOptionsFromTablePseudoconstant($pseudoconstant, $params, ($fieldSpec['localize_context'] ?? NULL), $context);
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the brackets are to tease @colemanw ? He he.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you see he asked me to put brackets BACK IN in a PR today!

Copy link
Contributor

Choose a reason for hiding this comment

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

I did. I admit I would have had to look up the precedence. I didn't, so I still don't know.

Copy link
Member

Choose a reason for hiding this comment

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

I'll take the brackets over CRM_Utils_Array any day!

@demeritcowboy
Copy link
Contributor

The $params array gets changed in the original before being passed to the hook but then because there's no ampersand it doesn't get changed in the refactored function. Seems like it could be a problem?

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I have added the ampersand

@demeritcowboy
Copy link
Contributor

Maybe we should just always add an ampersand to function params just in case... 😁

@eileenmcnaughton
Copy link
Contributor Author

aaarrrrghhhh

demeritcowboy
demeritcowboy reviewed Mar 26, 2020
View reviewed changes
CRM/Core/PseudoConstant.php Outdated
// Localize results
if (!empty($params['localize']) || $pseudoconstant['table'] === 'civicrm_country' || $pseudoconstant['table'] === 'civicrm_state_province') {
$I18nParams = [];
if ($localizeContent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a sneaky change but is ok. localizeContext vs. localizeContent. At least it has the proper 'z'. Yeah I said it. I can't hit people playing hockey anymore so this is as close as it gets to sport now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I flipped it back - your point

@eileenmcnaughton eileenmcnaughton mentioned this pull request Mar 26, 2020
Merged
@eileenmcnaughton
[REF] Extract code used to render a pseudoconstant when a table is de…
b53c346 
…fined.

This permits us to re-use for settings
@eileenmcnaughton
Copy link
Contributor Author

test this please

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • Don't see anything obvious. The output seem to have the same values before and after.
      • This 'table' variable is weird in that it only seems to come up for three tables, at least for the screens I visited, and they don't seem to have anything to do with the visited screens, so I'm not sure exactly where to go to see what this would affect.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] N/A
    • [r-test] PASS
      • The current failure has been noted elsewhere.

@mattwire
Copy link
Contributor

Test fail unrelated (conformance). Merging based on @demeritcowboy review

@mattwire mattwire merged commit ad85782 into civicrm:master Mar 27, 2020
@eileenmcnaughton eileenmcnaughton deleted the setting_ex branch March 28, 2020 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@demeritcowboy demeritcowboy demeritcowboy left review comments

@colemanw colemanw colemanw left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eileenmcnaughton @demeritcowboy @mattwire @colemanw