-
-
Notifications
You must be signed in to change notification settings - Fork 825
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 copyCustomFields function from Event to Core_DAO for re-usability #14171
Conversation
(Standard links)
|
@eileenmcnaughton this would break https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/BAO/Event.php#L989 though now that the function has been moved. |
@yashodha - yeah - I guess it makes sense to make non-static - I just did that But I realise the copy custom fields shouldn't be needed as it's also in generic. I think what happened is it got fixed in event first in an adhoc way and then fixed generically in copyGeneric but we still have this adhoc stuff hanging around cos it wasn't done right the first time & now we have to clean up. However the attempt was in 2016 & we have been raising the bar - I don't think that PR would have been accepted today without unit tests - #9407 |
@yashodha I added a second fix that brings back the genericness - hmm just checking it actually OK that worked - it looks like with that PR merged then we actually only need to do this --- a/CRM/Core/DAO.php
To make the custom fields work - I used the test from #13470 but without the tag handling as that would need revisiting -so the test looks like
|
Merging as test to follow in subsequent PR |
Overview
Minor refactor - move function per #13470
Before
Less readable
After
More readable
Technical Details
Note this is JUST a file move to keep it reviewable - I think the function should not be static & some genericisation should happen
Comments