Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-52544

Oracle segfaulting on php7 driver

    Details

      Description

      This happens during install, but also if you can get phpunit installed with php5 driver, then try and run on php7:

      vendor/bin/phpunit --debug lib/dml/tests/dml_test.php 
      /Users/danp/moodles/i30_oracle/moodle/lib/dml/oci_native_moodle_database.php on line 175
      Moodle 3.0+ (Build: 20151209), oci, 74c19577c9a94ddfada9b12becc6b2565858860d
      PHPUnit 4.8.10 by Sebastian Bergmann and contributors.
       
       
      Starting test 'core_dml_testcase::test_diagnose'.
      .
      Starting test 'core_dml_testcase::test_get_server_info'.
      .
      Starting test 'core_dml_testcase::test_get_in_or_equal'.
      .
      Starting test 'core_dml_testcase::test_fix_table_names'.
      .
      Starting test 'core_dml_testcase::test_fix_sql_params'.
      .
      Starting test 'core_dml_testcase::test_strtok'.
      .
      Starting test 'core_dml_testcase::test_tweak_param_names'.
      Segmentation fault: 11
      

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              poltawski Dan Poltawski added a comment -

              Process 36170 launched: '/usr/local/bin/php' (x86_64)
              Moodle 3.0+ (Build: 20151209), oci, 74c19577c9a94ddfada9b12becc6b2565858860d
              PHPUnit 4.8.10 by Sebastian Bergmann and contributors.
               
              ....Process 36170 stopped
              * thread #1: tid = 0x20bc6b, 0x00000001015a6dd9 oci8.so`php_oci_bind_post_exec + 73, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x11)
                  frame #0: 0x00000001015a6dd9 oci8.so`php_oci_bind_post_exec + 73
              oci8.so`php_oci_bind_post_exec:
              ->  0x1015a6dd9 <+73>: movq   0x10(%rbx), %rax
                  0x1015a6ddd <+77>: testq  %rax, %rax
                  0x1015a6de0 <+80>: je     0x1015a769a               ; <+2314>
                  0x1015a6de6 <+86>: cmpb   $0x0, 0x18(%rbx,%rax)
              

              Show
              poltawski Dan Poltawski added a comment - Process 36170 launched: '/usr/local/bin/php' (x86_64) Moodle 3.0+ (Build: 20151209), oci, 74c19577c9a94ddfada9b12becc6b2565858860d PHPUnit 4.8.10 by Sebastian Bergmann and contributors.   ....Process 36170 stopped * thread #1: tid = 0x20bc6b, 0x00000001015a6dd9 oci8.so`php_oci_bind_post_exec + 73, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x11) frame #0: 0x00000001015a6dd9 oci8.so`php_oci_bind_post_exec + 73 oci8.so`php_oci_bind_post_exec: -> 0x1015a6dd9 <+73>: movq 0x10(%rbx), %rax 0x1015a6ddd <+77>: testq %rax, %rax 0x1015a6de0 <+80>: je 0x1015a769a ; <+2314> 0x1015a6de6 <+86>: cmpb $0x0, 0x18(%rbx,%rax)
              Hide
              poltawski Dan Poltawski added a comment -

              Attaching trace of the run

              Show
              poltawski Dan Poltawski added a comment - Attaching trace of the run
              Hide
              poltawski Dan Poltawski added a comment - - edited

              Have been making test_tweak_param_names() simpler and simpler to try and get a basic testcase, and even this fails on the insert_record():

                  public function test_tweak_param_names() {
                      $DB = $this->tdb;
                      $dbman = $this->tdb->get_manager();
               
                      $table = $this->get_test_table();
                      $tablename = $table->getName();
               
                      $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
                      $table->add_field('columnname', XMLDB_TYPE_INTEGER, '2');
                      $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
               
                      $dbman->create_table($table);
               
                      // Test insert record.
                      $rec1 = new stdClass();
                      $rec1->columnname = 1;
               
                      // Insert_record().
                      $rec1->id = $DB->insert_record($tablename, $rec1);
                  }
              

              Show
              poltawski Dan Poltawski added a comment - - edited Have been making test_tweak_param_names() simpler and simpler to try and get a basic testcase, and even this fails on the insert_record(): public function test_tweak_param_names() { $DB = $this->tdb; $dbman = $this->tdb->get_manager();   $table = $this->get_test_table(); $tablename = $table->getName();   $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); $table->add_field('columnname', XMLDB_TYPE_INTEGER, '2'); $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));   $dbman->create_table($table);   // Test insert record. $rec1 = new stdClass(); $rec1->columnname = 1;   // Insert_record(). $rec1->id = $DB->insert_record($tablename, $rec1); }
              Hide
              poltawski Dan Poltawski added a comment -

              I've found that the segfault occurs on the returning part of it. If I comment out this:

                if ($returning) {
                          oci_bind_by_name($stmt, ":oracle_id", $id, 10, SQLT_INT);
                      }
              

              Then no segfault occurs. However, when tryign to reproduce the issue by running the same code:

              <?php
               
              $c = oci_connect('system', 'oracle', 'localhost:49161/XE');
               
               
              $s = oci_parse($c, "INSERT INTO u_test_table (foo) VALUES (:foo) RETURNING id INTO :oracle_id");
              $id = 0;
              $foo = 1;
              oci_bind_by_name($s, ':foo', $foo);
              oci_bind_by_name($s, ':oracle_id', $id, 10, SQLT_INT);
              oci_execute($s);
              

              Can't reprorudce the error.

              Show
              poltawski Dan Poltawski added a comment - I've found that the segfault occurs on the returning part of it. If I comment out this: if ($returning) { oci_bind_by_name($stmt, ":oracle_id", $id, 10, SQLT_INT); } Then no segfault occurs. However, when tryign to reproduce the issue by running the same code: <?php   $c = oci_connect('system', 'oracle', 'localhost:49161/XE');     $s = oci_parse($c, "INSERT INTO u_test_table (foo) VALUES (:foo) RETURNING id INTO :oracle_id"); $id = 0; $foo = 1; oci_bind_by_name($s, ':foo', $foo); oci_bind_by_name($s, ':oracle_id', $id, 10, SQLT_INT); oci_execute($s); Can't reprorudce the error.
              Hide
              poltawski Dan Poltawski added a comment -

              Noting that this still occurs on php 7.0.1

              Show
              poltawski Dan Poltawski added a comment - Noting that this still occurs on php 7.0.1
              Hide
              matteo Matteo Scaramuccia added a comment -
              Show
              matteo Matteo Scaramuccia added a comment - And probably 7.0.2: http://php.net/ChangeLog-7.php#7.0.2 .
              Hide
              poltawski Dan Poltawski added a comment -

              I've tested on 7.0.5, same issue.

              Show
              poltawski Dan Poltawski added a comment - I've tested on 7.0.5, same issue.
              Hide
              damyon Damyon Wiese added a comment -

              The first part if the fix seems to be:

              — a/lib/dml/oci_native_moodle_database.php
              +++ b/lib/dml/oci_native_moodle_database.php
              @@ -922,7 +922,7 @@ class oci_native_moodle_database extends moodle_database

              { return true; }
              • protected function bind_params($stmt, array $params=null, $tablename=null) {
                + protected function bind_params($stmt, array & $params=null, $tablename=null) {
                $descriptors = array();
                if ($params) {
                $columns = array();

              Because $params is modified in this function - it is converted to a local variable which is no longer in scope by the time oci_execute happens - boom.

              This is not a perfect fix because I still get: ORA-24816: Expanded non LONG bind data supplied after actual LONG or LOB column [NULL]

              Show
              damyon Damyon Wiese added a comment - The first part if the fix seems to be: — a/lib/dml/oci_native_moodle_database.php +++ b/lib/dml/oci_native_moodle_database.php @@ -922,7 +922,7 @@ class oci_native_moodle_database extends moodle_database { return true; } protected function bind_params($stmt, array $params=null, $tablename=null) { + protected function bind_params($stmt, array & $params=null, $tablename=null) { $descriptors = array(); if ($params) { $columns = array(); Because $params is modified in this function - it is converted to a local variable which is no longer in scope by the time oci_execute happens - boom. This is not a perfect fix because I still get: ORA-24816: Expanded non LONG bind data supplied after actual LONG or LOB column [NULL]
              Hide
              damyon Damyon Wiese added a comment -

              This patch lets you install at least - fails in phpunit though.

              Show
              damyon Damyon Wiese added a comment - This patch lets you install at least - fails in phpunit though.
              Hide
              damyon Damyon Wiese added a comment -

              OK Done - unit tests passing for me now on php7 + oracle.

              Show
              damyon Damyon Wiese added a comment - OK Done - unit tests passing for me now on php7 + oracle.
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-52544 using repository: git://github.com/damyon/moodle.git MOODLE_30_STABLE (1 errors / 0 warnings) [branch: MDL-52544-30 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , MOODLE_31_STABLE (1 errors / 0 warnings) [branch: MDL-52544-31 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , master (1 errors / 0 warnings) [branch: MDL-52544-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for working on this Damyon,

              Good finding

              Patch looks good to me, good to see now we are closing lobs properly. Just wondering if we should be using OCI-Lob::free as well.

              Would be nice to have a phpdoc for bind_params.

              Feel free to push it for integration when ready.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for working on this Damyon, Good finding Patch looks good to me, good to see now we are closing lobs properly. Just wondering if we should be using OCI-Lob::free as well. Would be nice to have a phpdoc for bind_params. Feel free to push it for integration when ready.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Adding Eloy Lafuente (stronk7) as watcher to this issue for his expert opinion.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Adding Eloy Lafuente (stronk7) as watcher to this issue for his expert opinion.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Got at Segmentation fault (core dumped) at around 64% phpunit + php7

              Show
              rajeshtaneja Rajesh Taneja added a comment - Got at Segmentation fault (core dumped) at around 64% phpunit + php7
              Hide
              damyon Damyon Wiese added a comment -

              We are already using oci_free_descriptor() which is the same as oci-lob::free.

              The manual page says: "Note:
              This function is commonly used as a method OCI-LOB::free.".

              Show
              damyon Damyon Wiese added a comment - We are already using oci_free_descriptor() which is the same as oci-lob::free. The manual page says: "Note: This function is commonly used as a method OCI-LOB::free.".
              Hide
              damyon Damyon Wiese added a comment -

              Segfault is here: ./vendor/bin/phpunit --stop-on-failure --debug --filter=auth_db_testcase::test_plugin

              Show
              damyon Damyon Wiese added a comment - Segfault is here: ./vendor/bin/phpunit --stop-on-failure --debug --filter=auth_db_testcase::test_plugin
              Hide
              poltawski Dan Poltawski added a comment - - edited

              Segfault is here: ./vendor/bin/phpunit --stop-on-failure --debug --filter=auth_db_testcase::test_plugin

              Sounds familiar: https://github.com/ADOdb/ADOdb/issues/170 (/MDL-52286) (anything related?)

              Show
              poltawski Dan Poltawski added a comment - - edited Segfault is here: ./vendor/bin/phpunit --stop-on-failure --debug --filter=auth_db_testcase::test_plugin Sounds familiar: https://github.com/ADOdb/ADOdb/issues/170 (/ MDL-52286 ) (anything related?)
              Hide
              damyon Damyon Wiese added a comment -

              The segfault is very weird - it's crashing in preg_match_all inside fix_table_names - I'm guessing something before hand is screwing with memory and the crash point is unrelated.

              Show
              damyon Damyon Wiese added a comment - The segfault is very weird - it's crashing in preg_match_all inside fix_table_names - I'm guessing something before hand is screwing with memory and the crash point is unrelated.
              Hide
              damyon Damyon Wiese added a comment -

              Yes I would guess it's an adodb bug corrupting the oci driver.

              Show
              damyon Damyon Wiese added a comment - Yes I would guess it's an adodb bug corrupting the oci driver.
              Hide
              rajeshtaneja Rajesh Taneja added a comment - - edited

              I was looking at the seg fault and it happening at preg_match_all() /home/rajesh/moodles/im/moodle/lib/dml/oci_native_moodle_database.php:300. So surely some kind of memory/stack problem.

              While looking at this area, there are 2 api where descriptors are not passed as done in other places.

              1. get_session_lock()
              2. release_session_lock()

              Also, not sure of the consequence but following passes the test and no seg fault

              diff --git a/auth/db/tests/db_test.php b/auth/db/tests/db_test.php
              index f1471cf..0ea449d 100644
              --- a/auth/db/tests/db_test.php
              +++ b/auth/db/tests/db_test.php
              @@ -68,7 +68,7 @@ class auth_db_testcase extends advanced_testcase {
                               break;
               
                           case 'oracle':
              -                set_config('type', 'oci8po', 'auth/db');
              +                set_config('type', 'oci8', 'auth/db');
                               set_config('sybasequoting', '1', 'auth/db');
                               break;
              

              or
              using the oci_fetch_array, instead of Obsolete ocifetchinto

              diff --git a/lib/adodb/drivers/adodb-oci8po.inc.php b/lib/adodb/drivers/adodb-oci8po.inc.php
              index 7c6049b..8ceba8b 100644
              --- a/lib/adodb/drivers/adodb-oci8po.inc.php
              +++ b/lib/adodb/drivers/adodb-oci8po.inc.php
              @@ -138,7 +138,7 @@ class ADORecordset_oci8po extends ADORecordset_oci8 {
                      // 10% speedup to move MoveNext to child class
                      function MoveNext()
                      {
              -               if(@OCIfetchinto($this->_queryID,$this->fields,$this->fetchMode)) {
              +               if ($this->fields = @oci_fetch_array($this->_queryID,$this->fetchMode)) {
                              global $ADODB_ANSI_PADDING_OFF;
                                      $this->_currentRow++;
                                      $this->_updatefields();
              

              Show
              rajeshtaneja Rajesh Taneja added a comment - - edited I was looking at the seg fault and it happening at preg_match_all() /home/rajesh/moodles/im/moodle/lib/dml/oci_native_moodle_database.php:300. So surely some kind of memory/stack problem. While looking at this area, there are 2 api where descriptors are not passed as done in other places. get_session_lock() release_session_lock() Also, not sure of the consequence but following passes the test and no seg fault diff --git a/auth/db/tests/db_test.php b/auth/db/tests/db_test.php index f1471cf..0ea449d 100644 --- a/auth/db/tests/db_test.php +++ b/auth/db/tests/db_test.php @@ -68,7 +68,7 @@ class auth_db_testcase extends advanced_testcase { break; case 'oracle': - set_config('type', 'oci8po', 'auth/db'); + set_config('type', 'oci8', 'auth/db'); set_config('sybasequoting', '1', 'auth/db'); break; or using the oci_fetch_array, instead of Obsolete ocifetchinto diff --git a/lib/adodb/drivers/adodb-oci8po.inc.php b/lib/adodb/drivers/adodb-oci8po.inc.php index 7c6049b..8ceba8b 100644 --- a/lib/adodb/drivers/adodb-oci8po.inc.php +++ b/lib/adodb/drivers/adodb-oci8po.inc.php @@ -138,7 +138,7 @@ class ADORecordset_oci8po extends ADORecordset_oci8 { // 10% speedup to move MoveNext to child class function MoveNext() { - if(@OCIfetchinto($this->_queryID,$this->fields,$this->fetchMode)) { + if ($this->fields = @oci_fetch_array($this->_queryID,$this->fetchMode)) { global $ADODB_ANSI_PADDING_OFF; $this->_currentRow++; $this->_updatefields();
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Raj - I submitted that patch for the ocipo driver upstream.

              https://github.com/ADOdb/ADOdb/pull/259

              Show
              damyon Damyon Wiese added a comment - Thanks Raj - I submitted that patch for the ocipo driver upstream. https://github.com/ADOdb/ADOdb/pull/259
              Hide
              damyon Damyon Wiese added a comment -

              Sending for integration - this is Raj's fix for AdoDB with my fix for our own driver.

              Show
              damyon Damyon Wiese added a comment - Sending for integration - this is Raj's fix for AdoDB with my fix for our own driver.
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-52544 using repository: git://github.com/damyon/moodle.git MOODLE_30_STABLE (1 errors / 1 warnings) [branch: MDL-52544-30 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , MOODLE_31_STABLE (1 errors / 1 warnings) [branch: MDL-52544-31 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , master (1 errors / 1 warnings) [branch: MDL-52544-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-52544 using repository: git://github.com/damyon/moodle.git MOODLE_30_STABLE (1 errors / 0 warnings) [branch: MDL-52544-30 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , MOODLE_31_STABLE (1 errors / 0 warnings) [branch: MDL-52544-31 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , master (1 errors / 0 warnings) [branch: MDL-52544-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              dmonllao David Monllaó added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              dmonllao David Monllaó added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-52544 using repository: git://github.com/damyon/moodle.git MOODLE_30_STABLE (1 errors / 0 warnings) [branch: MDL-52544-30 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , MOODLE_31_STABLE (1 errors / 0 warnings) [branch: MDL-52544-31 | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , master (1 errors / 0 warnings) [branch: MDL-52544-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              It's curious,

              I pretty much remember that we had to modify bind_params() and use, within the loop, $params[$key] everywhere instead of $value, because of some similar reason (a copy happens there). And that was enough under PHP5, the returned array was a different one (by value), but the array elements/values were ok (same instance, by reference).

              Maybe it was a PHP5 bug (or maybe it's a bug/new feature in PHP7, if you ask me that's my personal opinion), but it was enough in the past, who knows why it's not now..

              Anyway, really good catch, that way for sure we ensure both params and descriptors are always by ref and done. Same for the missing descriptor->close() call, that must be called always after writeTemporary(). Luckily, all the uses of descriptors we have in the driver are for such calls.

              About the NULL/empty string I'm not 100% sure which was the problem, maybe it's https://bugs.php.net/bug.php?id=72524 (only affects PHP7), in fact, I'm not 100% sold to this change as far as this changes data in a way it would not survive calling oracle_dirty_hack() twice and this may happen: NULL => 'empty string' => '1-whitespace string'. Surely I'd perform the conversion within bind_params() instead, I mean, as later as possible, and possibly with a call to that php issue so we can restitute NULLs once it's fixed (or at least have a reference to it).

              About ADOdb's changes, they look ok. My only comment is that, at some point, I think we should kill ADOdb completely, or at very least, provide the possibility of using our own drivers there in auth and enrol database plugins.

              Said that, I'm really a bit worried because some of the changes above are needed because a sudden change / ongoing bugs in php/oci behavior and that makes me feel that we'll be finding more (still hidden) obstacles in the road.

              I'm going to propose a extra commit with some of the points commented above, mainly clarifications, and let's discuss about them.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited It's curious, I pretty much remember that we had to modify bind_params() and use, within the loop, $params [$key] everywhere instead of $value , because of some similar reason (a copy happens there). And that was enough under PHP5, the returned array was a different one (by value), but the array elements/values were ok (same instance, by reference). Maybe it was a PHP5 bug (or maybe it's a bug/new feature in PHP7, if you ask me that's my personal opinion), but it was enough in the past, who knows why it's not now.. Anyway, really good catch, that way for sure we ensure both params and descriptors are always by ref and done. Same for the missing descriptor->close() call, that must be called always after writeTemporary() . Luckily, all the uses of descriptors we have in the driver are for such calls. About the NULL/empty string I'm not 100% sure which was the problem, maybe it's https://bugs.php.net/bug.php?id=72524 (only affects PHP7), in fact, I'm not 100% sold to this change as far as this changes data in a way it would not survive calling oracle_dirty_hack() twice and this may happen: NULL => 'empty string' => '1-whitespace string'. Surely I'd perform the conversion within bind_params() instead, I mean, as later as possible, and possibly with a call to that php issue so we can restitute NULLs once it's fixed (or at least have a reference to it). About ADOdb's changes, they look ok. My only comment is that, at some point, I think we should kill ADOdb completely, or at very least, provide the possibility of using our own drivers there in auth and enrol database plugins. Said that, I'm really a bit worried because some of the changes above are needed because a sudden change / ongoing bugs in php/oci behavior and that makes me feel that we'll be finding more (still hidden) obstacles in the road. I'm going to propose a extra commit with some of the points commented above, mainly clarifications, and let's discuss about them. Ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              I've prepared extra commit (on top of yours) fixing some small things:

              https://github.com/moodle/moodle/commit/8e9716f3b7b8004c3163675f1aac105009f219b1
              (https://github.com/moodle/moodle/compare/master...stronk7:MDL-52544)

              Other than that I'm happy with the changes, no matter:

              1) it makes me feel a bit nervous to change stables, but understand and agree it's needed.
              2) think there a small "excess" of free_descriptors() calls, specifically I think all get_xxxx() methods do not need them at all, only "write" ones do (LOBs in params are not used ever but there, in "write" statements). But in the other side, I don't see them breaking anything, so maybe it's better to have them always (no matter they are empty loops in practice).

              What do you think about the extra commit? If we agree I'm happy to push the 3 upstream...ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited I've prepared extra commit (on top of yours) fixing some small things: https://github.com/moodle/moodle/commit/8e9716f3b7b8004c3163675f1aac105009f219b1 ( https://github.com/moodle/moodle/compare/master...stronk7:MDL-52544 ) Other than that I'm happy with the changes, no matter: 1) it makes me feel a bit nervous to change stables, but understand and agree it's needed. 2) think there a small "excess" of free_descriptors() calls, specifically I think all get_xxxx() methods do not need them at all, only "write" ones do (LOBs in params are not used ever but there, in "write" statements). But in the other side, I don't see them breaking anything, so maybe it's better to have them always (no matter they are empty loops in practice). What do you think about the extra commit? If we agree I'm happy to push the 3 upstream...ciao
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Eloy +1 from me for the extra commit. As discussed in chat - I'm not wild about the php7 version check introduced - but you feel it is safer for existing sites so np.

              The free_descriptors calls are safe IMO - the descriptors only ever come from writeTemporary in this code - and if there are no writeTemporaries there are no descriptors. While strange, maybe there is some ugly code that is doing = on a long long string in a get_records and it could cause a very hard bug to track down if we don't consistently free all descriptors.

              Show
              damyon Damyon Wiese added a comment - Thanks Eloy +1 from me for the extra commit. As discussed in chat - I'm not wild about the php7 version check introduced - but you feel it is safer for existing sites so np. The free_descriptors calls are safe IMO - the descriptors only ever come from writeTemporary in this code - and if there are no writeTemporaries there are no descriptors. While strange, maybe there is some ugly code that is doing = on a long long string in a get_records and it could cause a very hard bug to track down if we don't consistently free all descriptors.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Thanks Damyon,

              integrated (30, 31 and master), yay!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Thanks Damyon, integrated (30, 31 and master), yay!
              Hide
              jpataleta Jun Pataleta added a comment -

              Sorry guys. I'm seeing the following on PHP7. (I'm running PHP 7.0.8 with oci 11.2.0.2.0 on Ubuntu 16.04)

              ................................E............................   61 / 5179 (  1%)
              ..E...............Segmentation fault (core dumped)
              

              I tried first using Raj's docker instance. Then I tried setting up Oracle XE 11g on my Windows 7 VM, but I also encountered these errors.

              Sorry, but I'll be failing for now to have this looked at.

              Show
              jpataleta Jun Pataleta added a comment - Sorry guys. I'm seeing the following on PHP7. (I'm running PHP 7.0.8 with oci 11.2.0.2.0 on Ubuntu 16.04) ................................E............................ 61 / 5179 ( 1%) ..E...............Segmentation fault (core dumped) I tried first using Raj's docker instance. Then I tried setting up Oracle XE 11g on my Windows 7 VM, but I also encountered these errors. Sorry, but I'll be failing for now to have this looked at.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Hi Jun,

              have you set oci8.statement_cache_size = 0 ? That's a known problem to run unit tests in Oracle (MDL-20339).

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Hi Jun, have you set oci8.statement_cache_size = 0 ? That's a known problem to run unit tests in Oracle ( MDL-20339 ).
              Hide
              jpataleta Jun Pataleta added a comment -

              No. Sorry I missed that. I set the statement_cache_size value and it seems good now. Please send it back for testing. Thanks!

              Show
              jpataleta Jun Pataleta added a comment - No. Sorry I missed that. I set the statement_cache_size value and it seems good now. Please send it back for testing. Thanks!
              Hide
              damyon Damyon Wiese added a comment -

              Juns fixed his oci8.ini file. Sending this back to testing.

              Show
              damyon Damyon Wiese added a comment - Juns fixed his oci8.ini file. Sending this back to testing.
              Hide
              jpataleta Jun Pataleta added a comment -

              Thanks Damyon and Eloy. All good now. All tests are passing.

              Tested on master, 31 and 30. Passing!

              Show
              jpataleta Jun Pataleta added a comment - Thanks Damyon and Eloy. All good now. All tests are passing. Tested on master, 31 and 30. Passing!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              With these awesome changes already spread to upstream repositories, this can be safely closed now. Many, many thanks for your hard work here!

              Programming is like sex. One mistake and you have to support it for the rest of your life.
              – Michael Sinz

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - With these awesome changes already spread to upstream repositories, this can be safely closed now. Many, many thanks for your hard work here! Programming is like sex. One mistake and you have to support it for the rest of your life. – Michael Sinz
              Hide
              marina Marina Glancy added a comment -

              Adding dev_docs_required label. Page https://docs.moodle.org/dev/Moodle_and_PHP7 still links to this issue as not resolved Oracle problem with PHP7

              Show
              marina Marina Glancy added a comment - Adding dev_docs_required label. Page https://docs.moodle.org/dev/Moodle_and_PHP7 still links to this issue as not resolved Oracle problem with PHP7
              Hide
              tsala Helen Foster added a comment -

              Please can anyone advise on what needs documenting in the user docs?

              I came across two pages about Oracle, one updated 2 years ago - https://docs.moodle.org/en/Installing_Oracle_for_PHP - and a short one which was updated 3 years ago - https://docs.moodle.org/31/en/Oracle

              Should either or both pages be deleted or redirected?

              Show
              tsala Helen Foster added a comment - Please can anyone advise on what needs documenting in the user docs? I came across two pages about Oracle, one updated 2 years ago - https://docs.moodle.org/en/Installing_Oracle_for_PHP - and a short one which was updated 3 years ago - https://docs.moodle.org/31/en/Oracle Should either or both pages be deleted or redirected?
              Hide
              poltawski Dan Poltawski added a comment -

              A good edit of those pages is definitely required I think. One thing that particularly needs updating with this issue is https://docs.moodle.org/dev/Moodle_and_PHP7#Can_I_use_PHP7_yet.3F

              (I would do it but about to leave)

              Show
              poltawski Dan Poltawski added a comment - A good edit of those pages is definitely required I think. One thing that particularly needs updating with this issue is https://docs.moodle.org/dev/Moodle_and_PHP7#Can_I_use_PHP7_yet.3F (I would do it but about to leave)
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              MDLQA-2505 Has been updated to ensure we run unit test with php 5.6 and php 7.x

              Show
              rajeshtaneja Rajesh Taneja added a comment - MDLQA-2505 Has been updated to ensure we run unit test with php 5.6 and php 7.x
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Note I was performing changes in that docs page related with SQL*Server and observed that Marina already had cleaned the comment about Oracle not working (pointing to this issue). So I would consider it done.

              About the install pages, I'd say all information there is valid, just this section is completely outdated for 31, because we require PHP 5.4 there (and the problems were with older php + extension versions). So I bet we can erase it without too much problem.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Note I was performing changes in that docs page related with SQL*Server and observed that Marina already had cleaned the comment about Oracle not working (pointing to this issue). So I would consider it done. About the install pages, I'd say all information there is valid, just this section is completely outdated for 31, because we require PHP 5.4 there (and the problems were with older php + extension versions). So I bet we can erase it without too much problem. Ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              (so, for me, forgot to say, it's ok to clean the docs_required label. only point is to delete the outdated section I commented above)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - (so, for me, forgot to say, it's ok to clean the docs_required label. only point is to delete the outdated section I commented above)
              Hide
              tsala Helen Foster added a comment -

              Thanks Eloy, I have deleted the outdated text as recommended and will remove the docs_required label now.

              Show
              tsala Helen Foster added a comment - Thanks Eloy, I have deleted the outdated text as recommended and will remove the docs_required label now.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Thanks!

                People

                • Votes:
                  1 Vote for this issue
                  Watchers:
                  9 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    12/Sep/16