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

Fix for #8034 - smart search fail on $ #8097

Merged
merged 1 commit into from May 8, 2016

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Oct 15, 2015

this fix for #8034
the problem was that $ is striped and the highlighter script tries to highlight the emptiness, that cause high resource usage by highlight.js

for testing please look the description in related issue 😉


Steps to reproduce the issue

  • enable smart search & smart search module
  • index content so it's not empty
  • search for "$" (single dollar sign)

Expected result

some search results

Actual result

javascript loop resulting in massive memory leak and eventual browser crash

System information (as much as possible)

browser = firefox
OS = Win 10

Additional comments

@Fedik Fedik mentioned this pull request Oct 15, 2015
@Fedik Fedik changed the title Fix for #8034 - do nothing when nothing to highlight Fix for #8034 - smart search fail on $ Oct 15, 2015
@alikon
Copy link
Contributor

alikon commented Oct 15, 2015

tested succesfully now no more loop and browser crash
not sure if is it out of scope of this pr but
something is still wrong :

wrong text $ sign is missed
smart search

wrong search result the $$$ is not matched/highlight
smart search

@Fedik
Copy link
Member Author

Fedik commented Oct 15, 2015

the problem was that $ is striped and the highlighter tried highlight emptiness, that cause high resource usage by highlight.js

@alikon
Copy link
Contributor

alikon commented Oct 15, 2015

i'm a js 🐤, just noticed that for search works as expected
search

surely @dgt41 can help us

@Fedik
Copy link
Member Author

Fedik commented Oct 15, 2015

$ is striped not in javascript, but somewhere in Smart Search component while handle the input ... I have not searched 😄

@chrisdavenport
Copy link
Contributor

I haven't checked the code, but I would think that all "punctuation" characters will be stripped. Same for the indexer. That would be expected behaviour.

@alikon
Copy link
Contributor

alikon commented Oct 16, 2015

@chrisdavenport Not so sure we can consider $ , € etc ..... a punctuation character
or at least not in all languages
so i think com_search is doing the correct behavior
what i'm missing?

@chrisdavenport
Copy link
Contributor

For the kind of websites I normally work with I wouldn't want these characters cluttering up the index. However, I can imagine that there would be occasions where some websites might want to retain them. I would suggest that this needs to be configurable because it depends on the particular application. It's rather like how we currently have a list of "stop words" that are not indexed. I think we should have a configurable list of "stop characters" that are discarded and/or regarded as delimiters. I'd be happy to review a PR for that feature.

@alikon
Copy link
Contributor

alikon commented Oct 21, 2015

👍

p.s.
i'm sorry for my shortsight, i usually work with ($, €) characters 👽
damned banking software work

@chrisdavenport
Copy link
Contributor

I have tested this item ✅ successfully on f3f7e62

The specific issue reported by @Fedik is fixed by this PR.

The consequential issue reported by @alikon should be the subject of a separate PR along the lines I suggested.


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

@zero-24
Copy link
Contributor

zero-24 commented May 8, 2016

I have tested this item ✅ successfully on f3f7e62

This specific issue is fixed. Thanks.


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

@zero-24
Copy link
Contributor

zero-24 commented May 8, 2016

RTC. Thanks


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 8, 2016
@brianteeman brianteeman added this to the Joomla 3.6.0 milestone May 8, 2016
@Kubik-Rubik
Copy link
Member

Thank you @Fedik and testers!

@Kubik-Rubik Kubik-Rubik merged commit 46a1b56 into joomla:staging May 8, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 8, 2016
@Fedik Fedik deleted the highlight-empty-fix branch May 8, 2016 14:45
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

7 participants