semsol / arc2

ARC RDF Classes for PHP
Other
331 stars 92 forks source link

Patch for issues 133 and 135 #138

Closed k00ni closed 4 years ago

k00ni commented 4 years ago

This pull request is fully based on the work of @craigdietrich.

k00ni commented 4 years ago

@craigdietrich, your changes seem to fail if mysqli is in use. With pdo adapter everything seems fine.

Example: https://travis-ci.org/semsol/arc2/jobs/657335341#L386

Any idea?

craigdietrich commented 4 years ago

Here is the way we've always hooked up, based on the ARC2 documentation:

$config = array(
  'db_host' => $hostname,
  'db_port' => $ci->db->port,
  'db_name' => $ci->db->database,
  'db_user' => $ci->db->username,
  'db_pwd'  => $ci->db->password,
  'store_name' => rtrim($ci->db->ARC_dbprefix, '_'),
);
$this->store =@ ARC2::getStore($config);
// $this->store->createDBCon();
if (!$this->store->isSetUp()) $this->store->setUp();
$this->ns = $ci->config->item('namespaces');

Is there a separate way to enable mysqli?

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 109


Files with Coverage Reduction New Missed Lines %
store/ARC2_StoreSelectQueryHandler.php 1 75.96%
store/ARC2_Store.php 3 74.66%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 105: 0.2%
Covered Lines: 2992
Relevant Lines: 7817

💛 - Coveralls
coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 109


Files with Coverage Reduction New Missed Lines %
store/ARC2_StoreSelectQueryHandler.php 1 75.96%
store/ARC2_Store.php 3 74.66%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 105: 0.2%
Covered Lines: 2992
Relevant Lines: 7817

💛 - Coveralls
k00ni commented 4 years ago

You already using mysqli, because its the default setting. It can be forced though by:

$config['db_adapter'] = 'mysqli';
k00ni commented 4 years ago

@craigdietrich: Can you please fix the code, which breaks tests in mysqli?

craigdietrich commented 4 years ago

I noticed that if the ARC tables are fresh (ie, have no data in them yet), if a predicate is asked for (SELECT) that has a question mark ("?") in the object, then this line in the 'joshcam' mysqli adapter throws a warning, because "$vals[2]" doesn't exist:

$val = $vals[$i++];

in

    protected function replacePlaceHolders($str, $vals)
    {
        $i = 1;
        $newStr = "";

        if (empty($vals)) {
            return $str;
        }

        while ($pos = strpos($str, "?")) {
            $val = $vals[$i++];
            if (is_object($val)) {
                $val = '[object]';
            }
            if ($val === null) {
                $val = 'NULL';
            }
            $newStr .= substr($str, 0, $pos) . "'" . $val . "'";
            $str = substr($str, $pos + 1);
        }
        $newStr .= $str;
        return $newStr;
    }

My changes allow queries through to id2val that it didn't before... not sure how this would relate to o2val, but the warning didn't exist before my changes went live. :/

k00ni commented 4 years ago

I lack insight here, sorry. @semsol do you have an idea?

craigdietrich commented 4 years ago

I think one think that would be helpful is for other(s) to check to see if their id2val tables are overrun with duplicate entries? (E.g., thousands of rows instead of dozens, depending on the ontologies being used.) MySQL versions and such could be in play but I hope I'm not the only one seeing the problem (in which case I need to get my eyes checked!).

Thanks!

k00ni commented 4 years ago

Which way do we go from here?

For me there are the following options:

  1. One has time to hunt down the bug, which causes this buggy behavior. A fix can be merged in, if all tests run without errors.
  2. We document all we have in the related issue(s) #133 resp. #135 and close this pull request for as long as there is no fix.

I won't merge in code which breaks our tests.

craigdietrich commented 4 years ago

2 makes sense, of course. Fortunate that nobody else is experience the is2val overrun!

k00ni commented 4 years ago

Closed for now until we got a fix, which doesn't break our tests.

@craigdietrich Thank you for your efforts anyway.