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

Added Tags filter for Joomla! articles #2561

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Kubik-Rubik
Copy link

This PR solves issues like #2534, #1631 and other.

Usage in the content-pro-joomla particle (as an example, use accordingly in other particles)

YAML file

article.filter.tags:
    type: input.text
    label: Tags
    description: Enter tag IDs to limit the articles that should be shown. It should be a list of tag IDs separated with a comma (i.e. 1,2,3,4,5).
    overridable: false

Twig file (add it before the find() line(!))

{# Tags #}
{% set tags_options = filter.tags ? {id: [filter.tags|replace(' ', '')|split(',')]} : {} %}
{% do article_finder.tags(tags_options) %}

Demo video: https://downloads.kubik-rubik.de/joomla/gantry5-tags-filter.mov

@mahagr
Copy link
Member

mahagr commented Nov 5, 2019

@Kubik-Rubik Thanks! I've been busy on other things and thus I've not had time to figure out how to integrate tags to the query. I do remember that tags were quite complicated as I did fix some issues with them years ago.

Do you have reference to the code which does the same in Joomla module? I want to be sure there isn't more into it which could cause unexpected behavior when using JOIN to tags table.

@mahagr mahagr self-requested a review November 5, 2019 08:47
@thexmanxyz
Copy link
Contributor

thexmanxyz commented Nov 27, 2019

@mahagr @Kubik-Rubik @yellowwebmonkey I verified the PR right now and it does work but TBH I don't like that the feature was implemented only based on the Tag ID which is quite difficult to handle for end users. Moreover like suggested in #2534 I would love to see the option to match ALL or ANY depending on a parameter. Nonetheless I will now add a commit to this PR which adds the functionality:

1.) to filter for both (id and name)
2.) to match ANY / ALL like suggested in #2534

Here is the PHP code to match for tag name instead of ID (just for the sake of completeness):

public function tags($tagNames= [])
{
    if (empty($tagNames['name'][0])) {
        return $this;
    }
    $tagIdsArray = array_map('intval', $tagNames['name'][0]);
    $this->query->join('INNER', '#__contentitem_tag_map AS t ON t.content_item_id = a.id');
    $this->query->join('INNER', '#__tags AS ta ON t.tag_id = ta.id');
    return $this->where('ta.title', 'IN', $tagIdsArray)->where('t.type_alias', '=', 'com_content.article');
}

@thexmanxyz
Copy link
Contributor

thexmanxyz commented Nov 27, 2019

@Kubik-Rubik Thanks! I've been busy on other things and thus I've not had time to figure out how to integrate tags to the query. I do remember that tags were quite complicated as I did fix some issues with them years ago.

Do you have reference to the code which does the same in Joomla module? I want to be sure there isn't more into it which could cause unexpected behavior when using JOIN to tags table.

@mahagr What unexpected behaviour do you have in mind? Actually there shouldn't be any unexpected side-effects because the tags are filtered by type_alias and joined correctly over the intended FK.

@Kubik-Rubik
Copy link
Author

Sorry for the late reply, was too busy!

I prefer the IDs more but we could add a second function for the Tag names to have both options.

@thexmanxyz Or do you want to do the commit that combines it all?

@thexmanxyz
Copy link
Contributor

thexmanxyz commented Nov 27, 2019

@Kubik-Rubik Don't worry! You were actually quite fast answering (~1 hour). I'm already in testing phase. I would not add a second method I will propose an update for 1.) in the next minutes. Stay tuned. I prefer to have everything within one method. Just define e.g.

{id: [ids], title: [titles]}

But don't bother it is already implemented and working. I will add my updated version of the tags() method to this PR.

@mahagr
Copy link
Member

mahagr commented Nov 27, 2019

I'm currently working on Gantry 5.5, this week I've been cleaning up the code for PHP 5.6/7.1. So if you can work on this one I would appreciate. :)

Can you check the code against Joomla code, to make sure we're using tags in the same way as they are?

I agree that we need support for working with both tag ids and names.

@thexmanxyz
Copy link
Contributor

thexmanxyz commented Nov 28, 2019

@Kubik-Rubik Can you please give me access to you fork? So that I can commit my changes directly? So I don't need to open another PR for the same thing and can directly push my changed to your repo. I would highly appreciate.

@thexmanxyz
Copy link
Contributor

thexmanxyz commented Nov 28, 2019

@mahagr @Kubik-Rubik @yellowwebmonkey @N8Solutions I'm finally done with the rework. The signature of the method now looks like this (pseudo-code):

tags({ids: [ids]|id, titles: [titles]|title}, matchAll = false)

  • ids: specify a list of tag ids or single id
  • titles: specify a list of tag titles or single title
  • matchAll: match for all specified tag titles or ids

1.) First and foremost I added the functionality to filter articles for both tag ids and titles. As already outlined in my previous comment. You can specify both of them and they are matched based on a SQL OR. So if a tag id is found for an article it will be returned as well as when the title matches.

2.) Additionally I added the functionality to allow users to execute a full match against tag ids as well as tag titles. That means, only articles are returned which full match a list of tag titles or tag ids.

To clarify how this works

If I have an article with the added tags tag1, tag2 and tag3 and I specify something like this: tags({titles: ['tag1', 'tag2']}, true). The article will be returned.

If I call the tags(...) method again with tags({titles: ['tag1', 'tag4']}, true). The article will not be returned because not all specified tags match the tags referenced by the article.

This does work interchangeably with both tag titles and ids. So if the titles don't match but the ids do match the article will be returned. This is also the same when omitting the matchAll parameter or setting matchAll = false.

Additional

I had to update the Finder.php as well as the ContentFinder.php file to prevent doubling of unnecessary code. E.g. I added a method which constructs a list of elements for the SQL IN keyword in the form of (id1, id2, id3) or ('string1', 'string2', 'string3'). The code was already there but I think it is useful to encapsulate that functionality within a protected method and be available within the class hierarchy.

Additionally I updated all quote(...) and quoteName(...) calls to omit accessing the $db connection directly which makes the code more readable and smaller. It's a facade and as the quote methods are quite frequently used this makes sense to me to create two separate methods within the Finder.php to skip accessing the $db connection directly. Everything looks now much sleeker. I think this is everything.

Finally, I will now check within the Joomla code and the usage of tags in general but I have no doubts that this is the way to go. But I will report back when I checked that. Additionally I will update the Gantry 5 documentation (gantry/docs#196) as well to reflect the changes and new functionality.

@mahagr
Copy link
Member

mahagr commented Nov 28, 2019

Let me know when you've pushed the updated code; your changes are not yet visible as I'm guessing you wanted to check it out before.

@thexmanxyz
Copy link
Contributor

thexmanxyz commented Nov 28, 2019

@mahagr I know, I had to wait on @Kubik-Rubik contribution invite. The changes are now pushed to the PR. You can now verify the changes. Everything is tested and everything I changed is included within the commit fe44122 (also including code comments).

@thexmanxyz
Copy link
Contributor

@Kubik-Rubik @mahagr I refined the .tags(...) method to be consistent with the already existent finder API.

I'm now using the plurals ids and titles for the parameters. Additionally I fixed a small bug in the original PR which resulted in a 2D array because in the initial commit you had to specify an array of ids in the form tags({ids: [[id1, id2, id3]]}) which overcomplicates things.

Finally I updated the API so that we can also specify single values instead of arrays only:
tags({ids: [ids]|id, titles: [titles]|title}, matchAll = false)

To support the conversion I created a method which takes care of single values / collections and converts them into an array. The routine was already used by the category(...) method. I think we can leave it now how it is. I also updated my previous post which contains the full explanation to reflect these changes as well.

@thexmanxyz
Copy link
Contributor

thexmanxyz commented Nov 28, 2019

@mahagr Here are the Joomla references for using tags (with joins). This is done the same way like we are doing this:

  • join #__content with #__contentitem_tag_map for getting tag ids here
  • join #__contentitem_tag_map with #__tags for getting tag titles here

I hope this is enough for verification. But what I noticed and what is currently missing within Gantry is $db->quoteName(...) for table names. Which is always present within Joomla. I will attach this to this PR at the positions were it is missing. I assume this is just for compatibility reason with some database system but we should follow this one as well since we are already touching the finder logic quite comprehensively.

Ad: quoteName(...) please check the Joomla documentation here.

@jameswadsworth
Copy link

This is so needed in Joomla! I hope after nearly 5 years this could be merge.

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.

4 participants