We launched new forums in March 2019—join us there. In a hurry for help with your website? Get Help Now!
    • 26876
    • 12 Posts
    The comments in this bug report - http://modxcms.com/bugs/task/866 - suggest that all raw mysql functions were replaced to use the DB reference in r2912 . However, in this release and also in the latest trunk version (r3138) the same raw mysql functions are still there. Was this reverted?

    I’m experiencing the same bug as the bug-reporter that you cannot connect to another database (using a new DBAPI object) without breaking the website (Error while inserting event log into database.).
    • They suggest they were changed in the two files affected by that commit. I suggest you post more information in this thread about the custom code you are authoring and the bug you are experiencing.
        • 26876
        • 12 Posts
        Ah yes. The commit indeed concerns two different files then the document.parser.class.inc.php. The original bug report mentioned code that came from this class (or at least the same code is used there), so I assumed the commit involved this as well. Sorry for this. No custom code is involved though.

        The code/function from document.parser.class.inc.php that causes the bug in the Snippets and also shows the use of raw mysql functions:
            # Add an a alert message to the system event log
            function logEvent($evtid, $type, $msg, $source= 'Parser') {
                $msg= mysql_escape_string($msg);
                $source= mysql_escape_string($source);
                $evtid= intval($evtid);
                if ($type < 1) {
                    $type= 1;
                }
                elseif ($type > 3) {
                    $type= 3; // Types: 1 = information, 2 = warning, 3 = error
                }
                $sql= "INSERT INTO " . $this->getFullTableName("event_log") . " (eventid,type,createdon,source,description,user) " .
                "VALUES($evtid,$type," . time() . ",'$source','$msg','" . $this->getLoginUserID() . "')";
                $ds= @mysql_query($sql);
                if (!$ds) {
                    echo "Error while inserting event log into database.";
                    exit();
                }
            }


        As the code part above shows, mysql_* functions are used while it should use $this->db->*. This also causes any snippet that creates a new database connection (either by making a new DBAPI instance or using mysql_* functions) to bug out the above mentioned function. The database connection that is used in the above function will use the newly created one.

        Replacing
        $ds= @mysql_query($sql);
        with
        $ds = $this->db->query($sql);
        fixes the snippet problem for now.

        I hope this provides enough information.
          • 27376
          • 576 Posts
          Here’s the complete patched function:
          <?php
              # Add an a alert message to the system event log
              function logEvent($evtid, $type, $msg, $source= 'Parser') {
                  $msg= $this->db->escape($msg);
                  $source= $this->db->escape($source);
                  $evtid= intval($evtid);
                  if ($type < 1) {
                      $type= 1;
                  }
                  elseif ($type > 3) {
                      $type= 3; // Types: 1 = information, 2 = warning, 3 = error
                  }
          	$values = array(
          		'eventid' => $evtid,
          		'type' => $type,
          		'createdon' => time(),
          		'source' => $source,
          		'description' => $msg,
          		'user' => $this->getLoginUserID()
          	);
          	$id = $this->db->insert($values, $this->getFullTableName('event_log'));
                  if (!$id) {
                      echo "Error while inserting event log into database.";
                      exit();
                  }
              }
          

          Jason, should I go through and convert ALL mysql_query calls in all files to use the DBAPI? It might fix problems like this...
          • This function was recently changed from using a dbapi function, because in certain situations, using the DBAPI would cause an infinite loop, causing thousands of events to be logged. The problem here is, users should make sure they do not hijack the existing database connection to connect to another database in their code. They must create a new connection and reference that connection in subsequent mysql_ calls to avoid this issue.

            If the DBAPI cannot create a unique connection to another database, then that needs to be addressed there, though I thought that change was made quite some time ago.
            • Actually, I’d prefer to see logEvent() go away or be completely changed not to write to the database. It’s problematic in that if the error is caused by being unable to connect to the database, writing the error to the database is, well, stupid.
              • And it might be better making a log file instead, since writing to the database can be problematic on overly busy servers, while you can generally get something written to a file. On the other hand, I’ve usually found the event log to be ... shall we say, less than helpful.
                  Studying MODX in the desert - http://sottwell.com
                  Tips and Tricks from the MODX Forums and Slack Channels - http://modxcookbook.com
                  Join the Slack Community - http://modx.org
                  • 26876
                  • 12 Posts
                  users should make sure they do not hijack the existing database connection
                  It’s not really the fault of the users. The mysql_* functions that are used in that function (and throughout that whole class) do not use the link_identifier parameter.

                  If the link identifier is not specified, the last link opened by mysql_connect() is assumed.
                  This is why the most recent mysql connection is used, causing logEvent to fail to write an entry in it’s own database. Even if DBAPI was used by the user, it internally uses mysql_connect. The problem lies in the use of the mysql_* functions in the document parser class.
                  • Quote from: Shadoze at Nov 16, 2007, 09:27 AM

                    If the link identifier is not specified, the last link opened by mysql_connect() is assumed.
                    This is why the most recent mysql connection is used, causing logEvent to fail to write an entry in it’s own database. Even if DBAPI was used by the user, it internally uses mysql_connect. The problem lies in the use of the mysql_* functions in the document parser class.
                    Yeah, I realized that after my first post Shadoze, but unfortunately, I do not have a solution, other than removing the database event log altogether.
                      • 26876
                      • 12 Posts
                      I’m not familiar enough with the MODx code, but if the logEvent function with the use of the DBAPI causes an infinite loop, then yes, the logEvent function should be removed, rewritten or otherwise taken care of (max number of call times?). I still believe though, that the use of DBAPI throughout every MODx class/script is the right approach.

                      because in certain situations, using the DBAPI would cause an infinite loop, causing thousands of events to be logged

                      Going over the code quickly, I noticed something that might be the cause of the infinite loop, when using the DBAPI in logEvent:


                      • 1. $modx->logEvent uses DBAPI
                      • 2. DBAPI can call $modx->messageQuit on error
                      • 3. $modx->messageQuit calls $modx->logEvent (goes to 1)

                      Note: $modx represents document.parser.class.inc.php