? Pending

User tests: Successful: Unsuccessful:

avatar sandrodz
sandrodz
18 Feb 2016

Reproduce issue:

  1. add unicode character tags on article edit screen e.g. ჯონი, test, example, bla (save).
  2. now write ჯონ to trigger ajax search
  3. inspect XHR response

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.

avatar sandrodz sandrodz - open - 18 Feb 2016
avatar sandrodz sandrodz - change - 18 Feb 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 18 Feb 2016
Labels Added: ?
avatar wojsmol
wojsmol - comment - 19 Feb 2016
avatar sandrodz
sandrodz - comment - 19 Feb 2016

@wojsmol thanks :)

avatar brianteeman brianteeman - change - 19 Feb 2016
Category Tags
avatar sandrodz
sandrodz - comment - 20 Feb 2016

@elinw Hi, remember this issue? we've discussed it in SO. I've finally fixed it the proper way. Would you help me push this PR?

avatar sandrodz
sandrodz - comment - 1 Mar 2016

so typical of J community. nobody gives a shit.

avatar infograf768
infograf768 - comment - 1 Mar 2016

Not sure I understand fully.

Do you mean you create a unique tag containing comas?
ჯონი, test, example, bla

If yes, do you expect, after patch to get this?
screen shot 2016-03-01 at 09 37 42

instead of
screen shot 2016-03-01 at 09 40 57

avatar sandrodz
sandrodz - comment - 1 Mar 2016

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.

avatar sandrodz
sandrodz - comment - 20 Apr 2016

anyone guys? this issue is dangling for too long, I'm even thinking to just remove this PR, since nobody cares.

avatar brianteeman
brianteeman - comment - 20 Apr 2016

Personally your comment of 1 march meant I wont be looking at it

avatar sandrodz
sandrodz - comment - 20 Apr 2016

doesn't your attitude exactly echo my comment of 1 march? :)

avatar chrisdavenport
chrisdavenport - comment - 8 May 2016

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.


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

avatar andrepereiradasilva andrepereiradasilva - test_item - 10 May 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 10 May 2016

I have tested this item :white_check_mark: successfully on 3309d17


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

avatar chrisdavenport chrisdavenport - test_item - 10 May 2016 - Tested unsuccessfully
avatar chrisdavenport
chrisdavenport - comment - 10 May 2016

I have tested this item :red_circle: 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.


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

avatar sandrodz
sandrodz - comment - 14 May 2016

@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.

avatar chrisdavenport
chrisdavenport - comment - 14 May 2016

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);

avatar roland-d
roland-d - comment - 24 Jun 2016

@sandrodz Any update on this issue?


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

avatar sandrodz
sandrodz - comment - 3 Aug 2016

@roland-d sorry, I'm not using Joomla anymore.

avatar brianteeman
brianteeman - comment - 3 Aug 2016

@chrisdavenport do you want to take this over and complete it?

avatar chrisdavenport
chrisdavenport - comment - 3 Aug 2016

Added to my to-do list.

avatar brianteeman
brianteeman - comment - 3 Aug 2016

Thanks @chrisdavenport shall I leave this one open?

avatar chrisdavenport
chrisdavenport - comment - 3 Aug 2016

@brianteeman Yes, please. I'm sure you'll bug me if it's still open in a month. ;-)

avatar brianteeman
brianteeman - comment - 3 Aug 2016

ok - will do

avatar chrisdavenport
chrisdavenport - comment - 23 Aug 2016

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.

avatar brianteeman
brianteeman - comment - 23 Aug 2016

@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/

avatar sandrodz
sandrodz - comment - 23 Aug 2016

Don't believe in magic.

avatar chrisdavenport
chrisdavenport - comment - 23 Aug 2016

@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.

avatar andrepereiradasilva
andrepereiradasilva - comment - 23 Aug 2016

you have select2 https://select2.github.io/examples.html that i think it's a fork of chosen

avatar chrisdavenport
chrisdavenport - comment - 23 Aug 2016

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.

avatar sandrodz
sandrodz - comment - 23 Aug 2016

Dear lord guys, I had this figured out. Let me see if I still have joomla container somewhere.

avatar roland-d
roland-d - comment - 4 Nov 2016

@chrisdavenport Any further progress on this?


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

avatar mbabker
mbabker - comment - 21 May 2017

Final call for updates or this PR will need to be closed as abandoned.

avatar franz-wohlkoenig franz-wohlkoenig - change - 22 May 2017
The description was changed
Status Pending Information Required
avatar joomla-cms-bot joomla-cms-bot - edited - 22 May 2017
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2017
Category com_tags com_tags Front End Libraries
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 27 May 2017

If this PR get no Response, it will be closed at 22th June 2017.

avatar chrisdavenport
chrisdavenport - comment - 27 May 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.

avatar franz-wohlkoenig franz-wohlkoenig - change - 27 May 2017
Status Information Required Discussion
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 27 May 2017

set Status back on "Discussion".


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

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 26 Oct 2017

@chrisdavenport any Update on this?


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

avatar BilalReffas
BilalReffas - comment - 23 Jul 2018

Do we still need this pull requests? I think the code was already refactored but someone @franz-wohlkoenig

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Jul 2018

@BilalReffas i don't know, a Dev can answer.

avatar roland-d
roland-d - comment - 23 Jul 2018

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.

avatar roland-d roland-d - close - 23 Jul 2018
avatar roland-d roland-d - change - 23 Jul 2018
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2018-07-23 13:25:56
Closed_By roland-d

Add a Comment

Login with GitHub to post a comment