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/financial#12 move soft credit item count to object property #12011

Merged
merged 1 commit into from May 16, 2018

Conversation

Projects
None yet
3 participants
@lcdservices
Copy link
Contributor

commented Apr 21, 2018

Overview

Move the soft credit row count from a hardcoded variable to a class property that can be modified via hook.

Before

Soft credit row count was hardcoded and couldn't be modified except through an override file.

After

Row count is now a class property that can be modified with the preProcess hook.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2018

Oh wow! Our code is full of terrifying things!

What worries me here is that it does expose another thing to the hooks - which you obviously intend to alter - which could change at any time. If you want this to be non-upgrade-breaky you should lock in your intended usage with a test

@eileenmcnaughton

This comment has been minimized.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

I'm going to merge this as basically harmless & slightly better coding practice. Even better would have been for it to be a protected property with a public getter & setter function & even better would have been a unit test to lock it in. Key thing is merging this is no guarantee it won't change in a later patch.

@eileenmcnaughton eileenmcnaughton merged commit e6c1a93 into civicrm:master May 16, 2018

1 check passed

default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.