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

getTagItemsQuery documentation incorrect, arguments causes SQL error #4698

Closed
wants to merge 1 commit into from
Closed

getTagItemsQuery documentation incorrect, arguments causes SQL error #4698

wants to merge 1 commit into from

Conversation

paul-muckypuddle
Copy link
Contributor

Changed 3rd arg of getTypes call to 'true' to lookup types by alias rather than ID (as per docs) (issue #4655)

Steps to reproduce the issue

1 - Call JHelperTags getTagItemsQuery method with the 2nd argument containing an array of type aliases (as perhttp://api.joomla.org/cms-3/classes/JHelperTags.html#method_getTagItemsQuery

Expected result

A DB object with items matching those tags, filtered by content type

Actual result

Error 1064 (MySQl error)

System information (as much as possible)

Joomla 3.3.6
PHP 5.3

Additional comments

The fix is fairly simple. getTagItemsQuery documentation says it expects the 2nd argument to be an array of content type aliases, a single alias, or null. In fact, because line 560 of JHelperTags calls self::getTypes('assocList', $typesr, false); with the 3rd param as 'false' it actually expects content type IDs, not aliases. Thus, the query causes an error. Changing this to true, or supplying getTagItemsQuery with type IDs, does not cause the issue

Changed 3rd arg of getTypes call to 'true' to lookup types by alias rather than ID (as per docs)
@jissues-bot jissues-bot changed the title Update tags.php getTagItemsQuery documentation incorrect, arguments causes SQL error Oct 16, 2014
@joomdonation
Copy link
Contributor

First of all, I must say that I don't have experience with Tag API (haven't built any extensions use Joomla core tags API yet). However, from a quick look at the code, I think you should correct the documentation instead of correcting the code, the reason is because:

  1. From the code of that method, it is clear that the method expect IDs (integer), not strings.

  2. In TagsModelTag class (Joomla core class), it pass that parameters as array of IDs as well, see the code at

    https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/models/tag.php#L196
    
    https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/models/tag.php#L145
    
    So if you fix the code here, you will need to fix the code in TagsModelTag as well.
    
  3. If there is any third party extensions use this API available at the moment, I believe if they use that method, they must pass the parameters in IDs as well (otherwise, it won't work). So if this PR is merged, the extensions which use this method will be broken (if they use that param).

I might be wrong (as I said, haven't used the API). So if you still want to do this change, maybe we need a developer who has strong experience with Tags API come here and confirm that it works.

@paul-muckypuddle
Copy link
Contributor Author

Makes sense to change the docs I guess.

@paul-muckypuddle
Copy link
Contributor Author

Whoops... didn't read that big button.

@brianteeman
Copy link
Contributor

As the issue is with the documentation I am asking that you please update the documentation and I am closing this issue

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4698.

@jissues-bot
Copy link

Set to "closed" on behalf of @brianteeman by The JTracker Application at issues.joomla.org/joomla-cms/4698

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants