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

[3.7.4] Falang database driver can't handle ->select('1') #17003

Closed
wants to merge 2 commits into from

Conversation

@olleharstedt
Copy link

commented Jul 6, 2017

Summary of Changes

As title.

Testing Instructions

Expected result

Actual result

Documentation Changes Required

@olleharstedt

This comment has been minimized.

Copy link
Author

commented Jul 6, 2017

I've notified Falang about this issue.

@Bakual

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2017

Using count() would need a GROUP statetement as well. But it looks useless anyway to run count when we don't need to count the rows.

That is something which Falang should fix as the core code is perfectly fine.

@olleharstedt

This comment has been minimized.

Copy link
Author

commented Jul 6, 2017

Using 1 AS x is another way to fix this issue, instead of COUNT(*).

@Bakual

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2017

I still think it's a bug in Falang which they need to fix.

@olleharstedt

This comment has been minimized.

Copy link
Author

commented Jul 6, 2017

I agree. Just saying, we won't be able to upgrade until either Falang or Joomla has changed. Or we make our own fork.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Jul 6, 2017

@olleharstedt can you please close this Issue if its not a Core Issue?

@olleharstedt

This comment has been minimized.

Copy link
Author

commented Jul 12, 2017

The following piece of code works differently in PHP 7 and PHP 5:

$mysqli = mysqli_connect("localhost", "root", "", "mydatabase");
$res = mysqli_query($mysqli, "select 1");
$row = mysqli_fetch_object($res);
var_dump($row);

In PHP 7 it will dump

object(stdClass)#3 (1) {
  [1]=>
  string(1) "1"
}

but in PHP 5

object(stdClass)#3 (0) {
}

Therefore I think the PR is motivated. Maybe it can be changed to select('id') instead of select('COUNT(*)') (only cosmetic change).

@olleharstedt olleharstedt reopened this Jul 12, 2017

@sbouey

This comment has been minimized.

Copy link

commented Jul 12, 2017

To complete this case.

Joomla don't see the problem because the loadResult work fine with select('1) but the loadObject don't work fine.
Falang use loadObject during the database override.

There are 2 file to change and replace for the check method
select('1')
by
libraries/joomla/table/asset.php line 109
->select($this->_db->quoteName('id'))

libraries/joomla/table/nested.php line 694
select($this->_db->quoteName($this->_tbl_key))

libraries/joomla/table/nested.php line 997
select($key)

@alikon

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

did'nt experienced php 5 vs php 7 different behaviour
can you test with mysqli_fetch_row instead of mysqli_fetch_object in your posted #17003 (comment) test code ?

..despite i think should be fixed on falang side...
we simply should avoid SELECT COUNT(*) when not needed

@sbouey

This comment has been minimized.

Copy link

commented Jul 12, 2017

With mysqli_fetch_row it's work but in Joomla if you change in the assets.php check method

if ($this->_db->setQuery($query, 0, 1)->loadResult())
by
if ($this->_db->setQuery($query, 0, 1)->loadObject())
this will not work on php 5.5 , 5.6

Falang use loadobject for all it's why there are a problem since the last Joomla version.

the change of the query
libraries/joomla/table/asset.php line 109
->select($this->_db->quoteName('id'))

fix the problem on falang , it's not a Joomla bug because Joomla use loadResult.

Stéphane

@olleharstedt

This comment has been minimized.

Copy link
Author

commented Jul 12, 2017

NB: SELECT COUNT(*) has no performance hit in this case, since it's always returning 0 or 1 row. SELECT id would work just as well, too.

@alikon

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

even if there are no performance issues, using count(*) is wrong imho we are asking for existence and not asking how many items there are

@sbouey

This comment has been minimized.

Copy link

commented Jul 13, 2017

Select (id) will work and it's a workaround for MySQL bug. no performance problem.

@alikon

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

there is no MySQL bug nor Joomla bug

@sbouey

This comment has been minimized.

Copy link

commented Jul 13, 2017

look at the @olleharstedt message and try this
with php 5.6 and php 7.0

$mysqli = mysqli_connect("localhost", "root", "", "mydatabase");
$res = mysqli_query($mysqli, "select 1");
$row = mysqli_fetch_object($res);
var_dump($row);

The result is not the same with an empty object for php 5

@alikon

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

Use key or id instead of 1 when selecting
This is needed for some third-party applications
due to the different behaviour in PHP versions for
mysqli_fetch_object.

@olleharstedt olleharstedt changed the title Falang database driver can't handle ->select('1') [3.7.4Falang database driver can't handle ->select('1') Jul 17, 2017

@olleharstedt olleharstedt changed the title [3.7.4Falang database driver can't handle ->select('1') [3.7.4] Falang database driver can't handle ->select('1') Jul 17, 2017

@sbouey

This comment has been minimized.

Copy link

commented Jul 19, 2017

The change proposed by olleharstedt don't affect performance and fix the prolem on Falang with php 5.
No problem with php 7

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2017

I am closing this as not a Joomla core issue.

@brianteeman brianteeman closed this Sep 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.