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

[REF] Move repeated code for Price Field labels into separate function #26375

Merged

Conversation

larssandergreen
Copy link
Contributor

@larssandergreen larssandergreen commented May 29, 2023

Overview

This is a first step towards some other improvements, but all this PR does is make this code more maintainable by moving a lot of repeated copy-pasted text into a separate function.

The code was not entirely identical in some places (for instance, before the pre and post help for selects had no spans around them, now they do, sold out markings missing on select options, now present), but I think these were oversights that were changed in one place and not in others. There is no significant functional change.

Comments

Lines 294-297 are now redundant for all cases except text fields. They'll be removed in the following PR but need to stay for now so that text fields keep working.

@civibot
Copy link

civibot bot commented May 29, 2023

(Standard links)

Comment on lines 804 to 805
$count = CRM_Utils_Array::value('count', $opt, '');
$max_value = CRM_Utils_Array::value('max_value', $opt, '');
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker but it would be really cool if you got rid of the deprecated CRM_Utils_Array::value calls while you're in here. That function is always a bit ambiguous but I'm guessing this is what it's trying to do:

Suggested change
$count = CRM_Utils_Array::value('count', $opt, '');
$max_value = CRM_Utils_Array::value('max_value', $opt, '');
$count = $opt['count'] ?? '';
$max_value = $opt['max_value' ?? '';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure! (I did this one before I learned about getting rid of those).

@eileenmcnaughton
Copy link
Contributor

Oh wow - good detective work - I had never spotted those parts of the case-from-hell were copy & paste

@colemanw
Copy link
Member

Code looks good to me. Anyone able to r-run?

@eileenmcnaughton
Copy link
Contributor

I can't see any difference...so I think this is a mergeable improvement.

The only thing I would (& might) change is to make the function protected and / or comment it to make it clear it is not supported for use outside of core. That is implicit anyway but making it explicit is better

image

image

@larssandergreen
Copy link
Contributor Author

Thanks @eileenmcnaughton and @colemanw. Here's the next refactor step for this.

@mlutfy
Copy link
Member

mlutfy commented Jul 18, 2023

Found a regression with Select fields: https://lab.civicrm.org/dev/core/-/issues/4439

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.

4 participants