Some changes and small bug fixes

[trabisdementia]

• Added @var for better code inspection
• Fixed Recent Faq Block option for displaying categories with no content
• Removed unnecessary duplicated queries

I know this is an RC so feel free to ignore this changes.
Thank you for the nice module!

[zyspec]

Thanks Trabis!

I'll look these over. I do have a couple of questions though...

1. Isn't the syntax for @var suppose to be something like /* @var XoopsObject $object */ instead of /* @var $object XoopsObject */ for code inspection?
2. Is there a reason you didn't use the Criteria class in the getCategoriesIdsWithContent() method? It appears the code in the new method does the same thing, just moves it to the XoopsfaqContents class - which is a good thing. I think there's a couple of additional lines of code that can now be removed in the blocks/xoopsfaq_category.php file too. Looks to me like we should be able to use something like:

/**
 * Returns category ids of categories that have content
 */
public function getCategoriesIdsWithContent()
{
    $ret      = array();
    $fields   = array('contents_cid');
    $criteria = new Criteria('contents_active', XoopsfaqConstants::ACTIVE);
    $criteria->setGroupBy('contents_cid');
    $results = $this->getAll($criteria, $fields, false);
    if (false === $results) {
        return $ret;
    }
    foreach ($results as $result) {
        $ret[$result['contents_cid']] = $result['contents_cid'];
    }

    return $ret;
}

Thoughts?

[trabisdementia]

Hi, 1 - The order is irrelevant, at least for me when using phpstprm. 2- I did not used the getall method because it appends the content_id in the list of fields and causes a strict error in my version of mysql. See: https://craftcms.stackexchange.com/questions/12084/getting-this-sql-error-group-by-incompatible-with-sql-mode-only-full-group-by Sent by phone. :) Em 19/05/2017 01:11, "zyspec" <notifications@github.com> escreveu:

…

Thanks Trabis! I'll look these over. I do have a couple of questions though... 1. Isn't the syntax for @var <https://github.com/var> suppose to be something like /* @var XoopsObject $object */ instead of /* @var $object XoopsObject */ for code inspection? 2. Is there a reason you didn't use the Criteria class in the getCategoriesIdsWithContent() method? It appears the code in the new method does the same thing, just moves it to the XoopsfaqContents class - which is a good thing. I think there's a couple of additional lines of code that can now be removed in the *blocks/xoopsfaq_category.php* file too. Looks to me like we should be able to use something like: /** * Returns category ids of categories that have content */public function getCategoriesIdsWithContent(){ $ret = array(); $fields = array('contents_cid'); $criteria = new Criteria('contents_active', XoopsfaqConstants::ACTIVE); $criteria->setGroupBy('contents_cid'); $results = $this->getAll($criteria, $fields, false); if (false === $results) { return $ret; } foreach ($results as $result) { $ret[$result['contents_cid']] = $result['contents_cid']; } return $ret;} Thoughts? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGFR7__Ki6JlazYoY2woUtnszlH7d2o1ks5r7N4qgaJpZM4Nfx_w> .

[mambax7]

Trabis, when you click on the variable $xfCatHandler in

$xfCatHandler = $xfHelper->getHandler('category');

with Alt+Enter, you'll get:
/** @var TYPE_NAME $xfCatHandler */

which then becomes:
/** @var XoopsfaqCategoryHandler $xfCatHandler */

Since this is already built-in in PhpStorm, for consistency I would suggest to follow that style

[trabisdementia]

Thank you Mamba, did not know about that. I have rearranged the docs now :)

[zyspec]

Thanks guys...

@trabisdementia ,

The SQL problem is good to know - I'm pretty sure that is, or will be, a problem in several other modules. What if I just add 'content_id' to the $fields() array in the call to the getAll() method so SQL expects it to return the column? That should make SQL happy and it's one less thing to remember to change when we transition this to using the XOOPS 2.6 dB access methods. Thoughts?

[trabisdementia]

@zyspec,
Adding content_id to the list of fields will not change anything, as I said, if you do not pass the content_id, the getAll method will append it anyway. The problem arises only when you need to use "group by". You can only "group by" the fields that you have selected. In this case, you would have to use group by 'content_id', 'content_pid'. Since I did not want to group by the content_id, I had to create a method that does not inject it in the list of selected fields.
Example:
select * from table group by col1 == bad
select col1 from table group by col1 == good
select col1, col2 from table group by col1 == bad (and this is what happens when using getAll
select col1, col2 from table group by col1, col2 == good

[zyspec]

Ok, now I get it - sorry. I had to re-read your post from earlier. The problem is also explained in the 12.19.3 MySQL Handling of GROUP BY Documentation. Looks like the only way around this would be to get rid of setGroupBy() and return everything. Cycling through the foreach() loop is then equivalent to performing an array_unique() on the results since it'll just overload the $ret key each time the 'contents_cid' is duplicated. Something like:

/**
 * Returns category ids of categories that have content
 */
public function getCategoriesIdsWithContent()
{
    $ret      = array();
    $fields   = array('contents_cid');
    $criteria = new Criteria('contents_active', XoopsfaqConstants::ACTIVE);
    $results = $this->getAll($criteria, $fields, false);
    if (false === $results) {
        return $ret;
    }
    foreach ($results as $result) {
        $ret[$result['contents_cid']] = $result['contents_cid'];
    }
    return $ret;
}

I like your solution for this better until we get to Doctrine, etc. in XOOPS 2.6. Returning all of the 'contents' from a dB call doesn't seem like a great idea (especially if there's a lot of FAQs in the dB).