Nice catch :)

Seems that this was already fixed. The patch cannot be applied.

Howcome? Yesterday I've submitted the PR. Can you point me to the respective commit or PR?

I was looking at the master branch :$

But thanks to the error I found that we have an issue in both 2.5.x and 3.x. The setQuery isn't called at all in both branches:

$query = $dbo->getQuery(true);
$query->select('COUNT(*)');
$query->from('#__menu');
$query->where('parent_id = ' . $active->id);
$query->where('published = 1');
$children = $dbo->loadResult();

should be:

$query = $dbo->getQuery(true);
$query->select('COUNT(*)');
$query->from('#__menu');
$query->where('parent_id = ' . $active->id);
$query->where('published = 1');

$dbo->setQuery($query);
$children = $dbo->loadResult();

This bug exists in both branches so if you want to fix it also for 3.x you are welcome :)

Be careful because in the 3.x branch the $dbo var is called $db

Thanks!

I am getting weird results when using $dbo->setQuery($query); Without it "seems" to be working too. I haven't look to deep in the classes.

Caution: Thinks might get working :P

It would be cool to take more advantage of chaining, as this not only reduces the overall code size, but also improves readability IMHO:

$children = $dbo->setQuery(
    $dbo->getQuery(true)
        ->select('COUNT(*)')
        ->from('#__menu')
        ->where('parent_id = ' . $active->id)
        ->where('published = 1')
)->loadResult();

Well if we are going to do it right I always prefer to get the query outside to be able to add a fast $query->dump() when needed. I also would use a try catch statement.

Something like:

$query = $dbo->getQuery(true)
    ->select('COUNT(*)')
    ->from('#__menu')
    ->where('parent_id = ' . (int) $active->id)
    ->where('published = 1');

try
{
    $children = $dbo->setQuery($query)->loadResult();
}
catch (Exception $e)
{
    throw new RuntimeException($e->getMessage());
}

I wasn't aware that there were right or wrong... to me it's more a matter of taste ;)

I agree that during development it might be handy to have the query object separated.

Catching a generic exception and re-throwing a RuntimeException does not make much sense to me, especially at this point of execution. Please enlighten me :)

A matter of taste that has nothing to do with the issue at hand ;)

It's the funny part of sharing code :)

Re-throwing exceptions makes no sense but that's what you end using when the first exception does nothing.

Now @vdespa can choose :D

@phproberto @elkuku Thanks for your feedback guys. I am just trying to patch the code as in is in the 2.5.x version. Making code improvements on this branch don't really help on the long run. The code needs to be consistent around all methods / classes, not just a single one. All this can be discussed and placed on the coding guideline for J.

Right now are around 20+ confirmed bugs in 2.5, not to mention ~4 pages. I would like to focus on them.

If you find any problems in the code as it is (like not working, possibly failing in under some conditions), let me know.

@vdespa as I told you there is no setQuery anywhere so the query is not getting executed.

Can you please convert that "seems to work" in a "it works"?

Thanks!

@phproberto I will look more deeply into it.

Fixed the problem with setQuery. Thanks for your feedback.

This has been fixed in both 2.5.x and staging in other commits and can be closed