CRM-17789 free result fixes #149
Conversation
…ectively clears the result
DB_<provider>->freeResult() only returns booleans so this function always returns TRUE
|
After some testing, this PR seems to make no difference in memory usage or functionality (despite the fact that mysql_free_result is never called when using a mysqli:// dsn, it doesn't make any difference in memory usage when running the tests). I will do some more testing to be sure this PR is obsolete |
| if (isset($_DB_DATAOBJECT['RESULTFIELDS'][$this->_DB_resultid])) { | ||
| unset($_DB_DATAOBJECT['RESULTFIELDS'][$this->_DB_resultid]); | ||
| } | ||
| if (isset($_DB_DATAOBJECT['RESULTS'][$this->_DB_resultid])) { | ||
| unset($_DB_DATAOBJECT['RESULTS'][$this->_DB_resultid]); | ||
| $_DB_DATAOBJECT['RESULTS'][$this->_DB_resultid]->free(); |
There was a problem hiding this comment.
Wasn't this just unset in the line before?
There was a problem hiding this comment.
Agree. Thus it won't build stating "Error: Call to a member function free() on a non-object"
|
@nielosz would you be interested in reviewing this PR? |
|
Does civi's core team provide a php 7 civicrm playground? Any chance to ssh onto it? |
|
A variety of issues remain before master branch for 4.7 will work on PHP7. But I take it you are asking whether buildkit or some other infrastructure more along the lines of a Docker container or a Vagrant image has been setup for testing of PHP7. @nganivet @totten any suggesions here? See dm-coding/civibuildkit-docker#1 |
|
@JoeMurray I imagine it would be quite easy to modify the Vagrant buildkit to include PHP7. Here the most obvious places where a change would be needed (change php5 package name to php7): Then you would have to check if any changes/patches are needed for composer, PHPUnit, mailcatcher and all other PHP-based utilities used by buildkit (and buildkit itself BTW). There also are PHPMyAdmin, PHPMemchachedAdmin, OpcacheStatus, PHP_CodeSniffer and a few others, but these are not needed for running buildkit or CiviCRM so less critical. That's all I see so far. So probably 1/2 day of work to get this done. |
|
This seems like a useful version to build since we will be moving to support PHP7 in the next few months. Let's see if djcf is interested in creating an appropriate Docker container for us. If not, might you have the time this week? |
|
Sorry Joe, I am not interested in PHP 7 anytime soon so will have to ------ Original Message ------
|
|
This has been resubmitted by Seamus in #169 to address the reported error. |
These fixes make it possible to for CRM_Core_DAO->freeResult() to be mysql(i) extension independent, and also fixes the DB_mysqli->freeResult to actually clear it.
This is needed for supporting PHP7 without php_mysql extension, so it doesn't break (existing) installations.