User tests: Successful: Unsuccessful:
Reproduce issue:
Result: you will see that response contains all terms ჯონი, test, example, bla (whole db).
This happens because, if unicode is used as tag search term trim($app->input->get('like', null)),
returns an empty string ""
. filter->clean trips on unicode characters and strips them out completely.
Solution is to not filter input at all, set as raw - now unicode characters return matched tags and not everything from DB.
Second commit fixes issue #8074, issue is apparent on sites with a lot of tags, they are all preloaded! imagine 80K tags hogging down browser... Since we have ajaxSearch why preload everything? Lets preload sane amount and rest will be pulled as ajaxsearch is triggered.
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Category | ⇒ | Tags |
so typical of J community. nobody gives a shit.
nope.
ჯონი, test, example, bla are all different tags. When you search for ჯონი you will get all tags from db in results because trim($app->input->get('like', null))
doesn't understand unicode characters.
anyone guys? this issue is dangling for too long, I'm even thinking to just remove this PR, since nobody cares.
Personally your comment of 1 march meant I wont be looking at it
doesn't your attitude exactly echo my comment of 1 march? :)
I can confirm the issue. I have tested your fix and it works, but in its current form would introduce an SQL injection vulnerability. However, if you replace "raw" with "string" that should provide sufficient input filtering without removing UTF8 characters. Please update your PR and it can be reviewed again. Thanks for reporting the issue.
I have tested this item successfully on 3309d17
I have tested this item unsuccessfully on 3309d17
The utf8 issue is fixed, but your change to libraries/cms/form/field/tag.php introduces a new bug: In an article that already has at least 1 tag set, you can't add any more tags.
I would recommend you remove your attempted fix for #8074 from this PR.
@chrisdavenport Hi, are you sure this bug is introduced by my fix? It seems more like a pre-existing js issue, that showed itself because we are not loading everything anymore.
I also cannot reproduce, gif would be helpful.
Steps to replicate the issue:
1. Given 2 tags: Red and Green, edit an article so it has just the Red tag.
2. Save and Close the article.
3. Edit the article again. Notice that the Green tag is no longer listed in the drop-down, so I can't select it.
I assume your intention is to list no more than 10 tags that have not already been selected, is that right? In that case you will need to count the number of tags already selected, add 10, then use that as the limit in the setQuery. I don't think you need the conditional. Perhaps something like:
$db->setQuery($query, 0, count($this->value) + 10);
@sandrodz Any update on this issue?
@chrisdavenport do you want to take this over and complete it?
Added to my to-do list.
Thanks @chrisdavenport shall I leave this one open?
@brianteeman Yes, please. I'm sure you'll bug me if it's still open in a month. ;-)
ok - will do
I've looked into this a bit further now. I can modify the initial page load to limit the number of tags in the list to some number, although my earlier suggestion won't work because it doesn't take into account the tags already selected (that can be fixed with a union select). However, there doesn't appear to be any option in chosen to push the required information into the Ajax call. What's needed is a way to include the list of tag ids already selected, together with the limit, into the Ajax request. I think it would require a significant hack to the chosen code and even if that were acceptable, it's beyond my present level of skill with JavaScript.
The problem with Unicode characters will most likely also need someone with more knowledge of JavaScript than me and a willingness to hack chosen.
I seem to recall there was some talk of replacing chosen with a more up-to-date package. Perhaps the replacement has the required capabilities, in which case this issue (or rather, pair of issues) can be revisited. Until then I suggest we just close it.
@chrisdavenport If my proposal to renew the version of chosen we are using
is accepted by the PLT maybe just maybe it will be magically fixed
On 23 August 2016 at 01:45, Chris Davenport notifications@github.com
wrote:
I've looked into this a bit further now. I can modify the initial page
load to limit the number of tags in the list to some number, although my
earlier suggestion won't work because it doesn't take into account the tags
already selected (that can be fixed with a union select). However, there
doesn't appear to be any option in chosen to push the required information
into the Ajax call. What's needed is a way to include the list of tag ids
already selected, together with the limit, into the Ajax request. I think
it would require a significant hack to the chosen code and even if that
were acceptable, it's beyond my present level of skill with JavaScript.The problem with Unicode characters will most likely also need someone
with more knowledge of JavaScript than me and a willingness to hack chosen.I seem to recall there was some talk of replacing chosen with a more
up-to-date package. Perhaps the replacement has the required capabilities,
in which case this issue (or rather, pair of issues) can be revisited.
Until then I suggest we just close it.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9155 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8Tr2tPAmvinXRr9v5qLVo26YlbIgks5qikLGgaJpZM4Hdj_D
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
Don't believe in magic.
@brianteeman The Unicode problem might have been fixed (maybe; dunno), but from my reading of their documentation, there is only a slender chance of fixing the limit problem. Maybe something can be done with chosen's events to fire off a separate Ajax request? Don't hold your breath.
Best bet is probably to fire off a feature request to the chosen maintainers. Or just ask them how to solve the problem and see what they say.
you have select2 https://select2.github.io/examples.html that i think it's a fork of chosen
Thinking about this some more, I think there is a way to do it. All we really need is a way to alter the Ajax URL to add a couple of extra query arguments. Looks like the URL is set here: https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/html/tag.php#L31
We would need to add something like &ids= and &limit= and be able to update them whenever a tag is selected/deselected.
Dear lord guys, I had this figured out. Let me see if I still have joomla container somewhere.
@chrisdavenport Any further progress on this?
Final call for updates or this PR will need to be closed as abandoned.
Status | Pending | ⇒ | Information Required |
Category | com_tags | ⇒ | com_tags Front End Libraries |
If this PR get no Response, it will be closed at 22th June 2017.
The issue is still valid (presumably, I haven't tested the latest versions) so this should remain open. We just need someone to spend some time coming up with a valid fix.
Status | Information Required | ⇒ | Discussion |
set Status back on "Discussion".
@chrisdavenport any Update on this?
Do we still need this pull requests? I think the code was already refactored but someone @franz-wohlkoenig
@BilalReffas i don't know, a Dev can answer.
I cannot reproduce the issue nor can @BilalReffas and @niklas-deworetzki-thm So I will assume this is resolved. If not, feel free to reopen this. Thanks everybody.
Status | Discussion | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-07-23 13:25:56 |
Closed_By | ⇒ | roland-d |
@sandrodz see sandrodz#1