Remove cloning hack from export and add unit tests #11703
Conversation
eileenmcnaughton
force-pushed
the
export
branch
from
ec4f44d
to
b9266b7
Compare
|
@lcdservices I tried to add a test on the other place where the clone hack is used in this - it appeared to have been added in conjunction with https://issues.civicrm.org/jira/browse/CRM-13995 so I tried to add a test to show that was working - but I concluded it is broken (prior to this change) as through both the UI and the unit test (eileenmcnaughton@14fb071) I was unable to get results that appeared correct to me - ie. there were 7 rows to export. 2 shared a household & neither of those were in the export |
|
@eileenmcnaughton test failure is related |
|
thanks - looks like data set up is failing due to some clean up issue - will look |
eileenmcnaughton
force-pushed
the
export
branch
from
b9266b7
to
94f3310
Compare
eileenmcnaughton
force-pushed
the
export
branch
from
94f3310
to
bab4f15
Compare
|
@eileenmcnaughton I ran through some tests with/without this PR and the results were identical. So in terms of cleaning up the hackish code, I think it's fine. However -- the actual behavior is not working as intended. But let's handle that in another issue. |
| // the convertToPseudoNames will not adequately over-write them either as it doesn't 'kick-in' unless the | ||
| // relevant property is set. | ||
| // It may be that a long-term fix could be introduced there - however, it's probably necessary to figure out how to test the | ||
| // export class before tackling a better architectural fix |
There was a problem hiding this comment.
@eileenmcnaughton is there some recent change in CRM_Core_DAO::convertToPseudoNames that makes the above comments moot? I see you removing the workaround here but not doing anything else to address the stated problem.
There was a problem hiding this comment.
@colemanw the workaround dates back to 4.4 - I tried replicating the described issue from when it was introduced and could not so I concluded that it had been otherwise resolved since then
|
@colemanw as the person who added the hack that this tackles - do you have any idea of how to confirm it's no longer needed? I didn't find it needed in the things I tried but.... |
Overview
Remove a hacky & obsolete fix from the code base and add unit tests
Before
DAO is being cloned in order to address a former bug in export. No tests
After
DAO cloning removed, tests added.
Technical Details
Back in 4.4 a bug (CRM-14398) was addressed by cloning the DAO. This causes an issue in conjunction with #11615 (which adds a __destruct to the DAO in place of $dao->free being needed & in place of the damaging freeResult() calls & resolves a memory leak). In UI and unit test testing I found that the cloning hack is no longer required (pseudoConstant handling was fairly new when it was added but has matured now).
Comments
I also set a default of is_active=1 on the campaign api, which is consistent with other api & was useful for the unit test