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

Remove '$dao->free()' part 1 #13192

Merged
merged 1 commit into from
Dec 3, 2018
Merged

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Nov 30, 2018

See https://lab.civicrm.org/dev/core/issues/562

First round: remove lines exactly matching '$dao->free();'

@civibot
Copy link

civibot bot commented Nov 30, 2018

(Standard links)

@civibot civibot bot added the master label Nov 30, 2018
@seamuslee001
Copy link
Contributor

@aydun can you rebase against current master please

@seamuslee001
Copy link
Contributor

Jenkins retest this please

@aydun
Copy link
Contributor Author

aydun commented Dec 3, 2018

@eileenmcnaughton @seamuslee001 This was a quick test - a bit of perl hackery adapted from previous mass changes to docs - to remove exact matches of '$dao->free();' under the CRM tree, and then see how the test suite reacted. The tests ran cleanly here although I wonder how much the tests would actually pick up any issues related to this.

How do you want to segment and test this? There are matches under other directories and places where the variable is not '$dao' All can be picked up by tweaking the perl regexp.

Maybe:

  • remove all '$dao->free()'
  • as above but variants of 'dao'
  • examine other lines calling '->free()' - but excluding calls to CRM_Core_Config::free()

Each stage requires progressively more review.

@eileenmcnaughton
Copy link
Contributor

@aydun I'm pretty confident about ripping these out - but I'd probably merge 3-4 files worth now & a few after the rc with an increasing pace - starting with higher traffic files like CRM/Core/DAO.php perhaps in the first round since it would be there is we did see something.

@aydun aydun changed the title WIP Remove '$dao->free()' part 1 Remove '$dao->free()' part 1 Dec 3, 2018
@aydun
Copy link
Contributor Author

aydun commented Dec 3, 2018

@eileenmcnaughton Ok, here's 4 for starters

@seamuslee001
Copy link
Contributor

Merging as per the tag

@seamuslee001 seamuslee001 merged commit ef60d30 into civicrm:master Dec 3, 2018
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jan 3, 2019
Per gitlab

The DAO object since ? 4.7.x? has been freed on _destruct. Using the ->free action has been demonstrated to create some rare bugs - ie. because query sets from the outer loop can be lost. There is no benefit in calling it any more and some harm

civicrm#13192 was the last one in this series
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants