[4.0] Decoupling highlighter plugin from com_finder #20571

Hackwar · 2018-05-24T17:33:01Z

This PR tries to fix #20252.

This PR introduces a new event, "onFinderResult", which takes a single result object and the search query object. This allows plugins to modify the results (for example changing MIME types, link types, adding additional data, etc.) and in this case it allows the highlight plugin to modify the URL of the link of the search result to add the highlight parameter. This decouples the highlight system plugin from com_finder.

The ```FinderIndexerResult``` object has both an ```url``` and ```route``` attribute and we are now using ```url``` for display purposes and ```route``` for the links href.
I also tried to change a few of the calls to the new namespaced versions.

[UPDATE]
In further reviews, I also found out the issue why searchwords with utf8 characters are not properly highlighted. It turns out that OutputFilter::stringJSSafe() is not utf8 aware and I had to rewrite it. Now it properly encodes this into a utf8 escaped string.

[UPDATE]
Yet another update regarding the ```url``` and ```route```: url is actually the ID of the item and thus can not be used. Instead I added ```cleanurl``` instead.

## How to test
* Test smart search and see the highlighting parameter in the URL of the search results.
* Apply the patch and see that it works as before, except that the additional highlight parameter is in the URL below the result.
* This PR mainly needs a codereview.

Quy · 2018-05-24T19:00:08Z

components/com_finder/View/Search/HtmlView.php

@@ -112,19 +116,19 @@ class HtmlView extends BaseHtmlView
 	 */
 	public function display($tpl = null)
 	{
-		$app    = \JFactory::getApplication();
-		$params = $app->getParams();
+		$app    = Factory::getApplication();


Remove extra spaces before = or align with =.

carlitorweb · 2018-05-24T19:12:06Z

plugins/system/highlight/highlight.php

+	 *
+	 * @return  void
+	 *
+	 * @since   4.0


change to __DEPLOY_VERSION__

carlitorweb · 2018-05-24T19:44:02Z

I got this. This PR depend of other?

Hackwar · 2018-05-24T19:52:31Z

No, but there is another bug that is fixed in #20185. Looks like this strict query handling was disabled again in a later version of 4.0-dev. Idk. Maybe update your 4.0-dev branch to the current state? It works there...

carlitorweb · 2018-05-24T19:56:37Z

I test now without this PR, and give the same error. So, is not this PR. And yes, the branch is up to the current state

carlitorweb · 2018-05-24T21:25:26Z

Ok, the error I get is fixed here #20185. Beside code review will be good test this, but we need #20185 merged for that

Quy · 2018-05-25T03:29:02Z

plugins/system/highlight/highlight.php

+	 *
+	 * @since   __DEPLOY_VERSION__
+	 */
+	public function onFinderResult($item, $query)


There is afterFinderResults. For consistency, make it onFinderResults with a s?

Yes, there is a afterFinderResults, but that is only the name of a mark in the logging, not a plugin event. Since I want to run the event on each result separately, I don't really would want to add the s to the event name...

…4finder_highlighter # Conflicts: # components/com_finder/View/Search/HtmlView.php

Hackwar · 2018-06-12T09:44:26Z

#20185 has been merged in here. Happy testing. 😄

carlitorweb · 2018-06-18T16:12:38Z

Got this:
Warning: Invalid argument supplied for foreach() in .....components\com_finder\tmpl\search\default_results.php on line 49

carlitorweb · 2018-06-18T16:15:46Z

components/com_finder/View/Search/HtmlView.php

+		$this->state = $this->get('State');
+		$this->query = $this->get('Query');
+		\JDEBUG ? Profiler::getInstance('Application')->mark('afterFinderQuery') : null;
+		$this->results = $this->get('Results');


Revert this one, is $this->get('Items')

carlitorweb · 2018-06-18T16:20:08Z

All work ok. Waiting for the fix of the error

Hackwar · 2018-06-18T17:00:31Z

This was a bug because of a conflict between the original code and #20185. In #20185 it was changed to "Items" and somehow that change was overwritten here. Should work now.

carlitorweb · 2018-06-18T17:38:40Z

I have tested this item ✅ successfully on a52bd58

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20571.}

Hackwar · 2018-06-23T18:23:07Z

I've updated the PR and fixed some things. It would be nice if you could test this again and if we could get one more tester for this. 😄

Quy · 2018-06-23T19:55:45Z

The URLs don't match where the only difference should be the highlight query string.

http://localhost/joomla-cms-j4finder_highlighter/index.php/blog/5-your-modules?highlight=WyJqb29tbGEiXQ==

http://localhost/joomla-cms-j4finder_highlighter/index.php/component/content/article/5-your-modules

Hackwar · 2018-06-23T20:04:34Z

The issue here is, that FinderIndexerAdapter::getURL() returns the wrong URL. That method should actually be deprecated... We have to fix the finder plugins for that.

Quy · 2018-06-23T20:09:54Z

components/com_finder/tmpl/search/default_result.php

-	$route .= '&highlight=' . base64_encode(json_encode($this->query->highlight));
-}
-
+$url = JRoute::_($this->result->route);


Add a blank line above. Or not assign it to a variable and leave it where it was before.

Hackwar · 2018-06-26T21:55:14Z

I've changed the way the URLs are produced, which should fix the issue that you had and also cleaned up the result layout. Please test again.

Hackwar · 2018-06-27T19:19:30Z

Ok, I got this right now. I removed the FinderIndexerHelper::getContentPath() method, since we don't need that with the new router anymore. I also cleaned up the finder plugins and improved the comments there. Last but not least, I added a cleanURL attribute after unserialising the object in the model, where I simply copy the route attribute to this. That allows us to render the clean URL in the result layout and at the same time allow the highlighter plugin to add to the URL.

So please test and have a go. 😄

Hackwar · 2018-06-27T21:15:04Z

Since @infograf768 requested this, I also reviewed the highlighting of search words with utf8 chars in the search results page. It turns out that \Joomla\CMS\Filter\OutputFilter::stringJSSafe() is not utf8 aware and mangles all characters that are not ASCII. I rewrote that and now it should work fine for utf8. Looking for your test, @infograf768 😉

infograf768 · 2018-06-28T06:27:03Z

@Hackwar
Indeed, the issue was whether in js or the method ( #20562 (comment) ).

You solved it here and utf8 is now correctly highlighted. 👍

Remains an issue here with terms surrounded by doublequotes or singlequotes. If one does not search WITH the quotes, i.e. for example 'frontale (term in text is 'frontale') then we have no suggestion. If I enter frontale no suggestion and no result but I am proposed to search for 'frontale'.
(That issue does not exist in com_search although highlighting is broken for com_search in 4.0. Works on 3.x)

Hackwar · 2018-06-28T08:53:46Z

The suggestions have nothing to do with highlighting. I hope no one now gets confused and delays this PR because you are bringing up unrelated bugs here.

infograf768 · 2018-06-28T09:02:21Z

I sincereley hope not. As usual I test a few things when I test any of your PRs. Just consider it as a note about something to remember if it can be solved or not.

As I only tested the highlighting and not the hardcoupling issue, I can't set the my test as OK on issues.joomla.org

carlitorweb · 2018-06-28T19:41:16Z

About search for specific phrases, we can continue this talk in #20384 , is where @Hackwar change this.

carlitorweb · 2018-06-28T19:57:44Z

I have tested this item ✅ successfully on 9834dd9

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20571.}

infograf768 · 2018-06-29T05:38:18Z

I have tested this item ✅ successfully on 9834dd9

@laoneo @mbabker
This PR is fine concerning the highlighting of terms. But I have no idea about the decoupling.

Can you have a look at that aspect and set RTC/Merge if OK?

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20571.}

laoneo · 2018-06-29T05:55:50Z

plugins/finder/categories/categories.php

@@ -304,8 +306,6 @@ protected function index(FinderIndexerResult $item, $format = 'html')
 			$item->route = ContentHelperRoute::getCategoryRoute($item->id, $item->language);
 		}

-		$item->path = FinderIndexerHelper::getContentPath($item->route);


Is this removal necessary then or can J3 plugins still work with that code in place?

Hackwar · 2018-06-29T06:03:22Z

Since this is more or less broken code and potentially creates wrong URLs, this code will not work in 4.0 anymore. Plugins from 3.x can't use it anymore and shouldn't use it now already.Am 29.06.2018 07:56 schrieb Allon Moritz <notifications@github.com>:@laoneo commented on this pull request. In plugins/finder/categories/categories.php:

@@ -304,8 +306,6 @@ protected function index(FinderIndexerResult $item, $format = 'html')

$item->route = ContentHelperRoute::getCategoryRoute($item->id, $item->language); } - $item->path = FinderIndexerHelper::getContentPath($item->route); Is this removal necessary then or can J3 plugins still work with that code in place? —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

laoneo · 2018-06-29T07:26:45Z

Is it then deprecated in J3?

Hackwar · 2018-06-29T07:29:24Z

I'm going to create a depreciation PR soon.Am 29.06.2018 09:26 schrieb Allon Moritz <notifications@github.com>:Is it then deprecated in J3? —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

Hackwar · 2018-06-29T19:56:34Z

PR for deprecation: #20934

carlitorweb · 2018-07-06T11:52:26Z

I have tested this item ✅ successfully on 55057f2

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20571.}

infograf768 · 2018-07-06T11:55:47Z

I have tested this item ✅ successfully on 55057f2

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20571.}

infograf768 · 2018-07-06T11:56:07Z

RTC. Thanks

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20571.}

Hackwar · 2018-07-06T13:35:28Z

Thank you!

Decoupling highlighter plugin from com_finder

ef08e72

Hackwar mentioned this pull request May 24, 2018

Hard coupling between com_finder and plg_system_highlight #20252

Closed

joomla-cms-bot added the PR-4.0-dev label May 24, 2018

Hackwar mentioned this pull request May 24, 2018

[4.0] Reworking result layout of com_finder #20572

Merged

Codestyle

0a32ecf

Quy reviewed May 24, 2018

View reviewed changes

carlitorweb suggested changes May 24, 2018

View reviewed changes

plugins/system/highlight/highlight.php Outdated

*

* @return void

*

* @since 4.0

Copy link

Member

carlitorweb May 24, 2018 •

edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to __DEPLOY_VERSION__

Hackwar added 2 commits May 24, 2018 21:22

Codestyle

c4e13c7

Version comment

e942322

Quy reviewed May 25, 2018

View reviewed changes

Merge branch '4.0-dev' of https://github.com/joomla/joomla-cms into j…

3d4e166

…4finder_highlighter # Conflicts: # components/com_finder/View/Search/HtmlView.php

carlitorweb suggested changes Jun 18, 2018

View reviewed changes

Fixing bug due to joomla#20185

a52bd58

Hackwar added 2 commits June 23, 2018 20:16

Using url for display and route for route-URL

30fa0a5

Merge branch '4.0-dev' into j4finder_highlighter

38f4885

Quy reviewed Jun 23, 2018

View reviewed changes

Merge branch '4.0-dev' into j4finder_highlighter

e2c05da

Cleaning up finder plugins and adding cleanURL attribute in model

acc8529

Hackwar force-pushed the j4finder_highlighter branch from 0a5631b to acc8529 Compare June 27, 2018 19:13

Making OutputFilter::stringJSSafe() utf8 aware

9834dd9

Hackwar mentioned this pull request Jun 27, 2018

[4.0] Fixing JS in com_finder #20888

Merged

laoneo reviewed Jun 29, 2018

View reviewed changes

Hackwar mentioned this pull request Jun 29, 2018

Deprecate FinderIndexerHelper::getContentPath() #20934

Merged

Hackwar added 2 commits July 1, 2018 18:56

Merge branch '4.0-dev' into j4finder_highlighter

53dd22f

Merge branch '4.0-dev' into j4finder_highlighter

55057f2

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 6, 2018

laoneo added this to the Joomla 4.0 milestone Jul 6, 2018

Merge branch '4.0-dev' into j4finder_highlighter

66e848b

laoneo merged commit 0650916 into joomla:4.0-dev Jul 6, 2018

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 6, 2018

Hackwar deleted the j4finder_highlighter branch October 23, 2020 19:58

[4.0] Decoupling highlighter plugin from com_finder #20571

[4.0] Decoupling highlighter plugin from com_finder #20571

Conversation

Hackwar commented May 24, 2018 • edited by infograf768

Quy May 24, 2018

Choose a reason for hiding this comment

carlitorweb May 24, 2018 • edited

Choose a reason for hiding this comment

carlitorweb commented May 24, 2018

Hackwar commented May 24, 2018

carlitorweb commented May 24, 2018 • edited

carlitorweb commented May 24, 2018

Quy May 25, 2018

Choose a reason for hiding this comment

Hackwar Jun 12, 2018

Choose a reason for hiding this comment

Hackwar commented Jun 12, 2018

carlitorweb commented Jun 18, 2018

carlitorweb Jun 18, 2018 • edited

Choose a reason for hiding this comment

carlitorweb commented Jun 18, 2018

Hackwar commented Jun 18, 2018

carlitorweb commented Jun 18, 2018

Hackwar commented Jun 23, 2018

Quy commented Jun 23, 2018

Hackwar commented Jun 23, 2018

Quy Jun 23, 2018

Choose a reason for hiding this comment

Hackwar commented Jun 26, 2018

Hackwar commented Jun 27, 2018

Hackwar commented Jun 27, 2018

infograf768 commented Jun 28, 2018

Hackwar commented Jun 28, 2018

infograf768 commented Jun 28, 2018

carlitorweb commented Jun 28, 2018

carlitorweb commented Jun 28, 2018

infograf768 commented Jun 29, 2018

laoneo Jun 29, 2018

Choose a reason for hiding this comment

Hackwar commented Jun 29, 2018 via email

laoneo commented Jun 29, 2018

Hackwar commented Jun 29, 2018 via email

Hackwar commented Jun 29, 2018

carlitorweb commented Jul 6, 2018

infograf768 commented Jul 6, 2018

infograf768 commented Jul 6, 2018

Hackwar commented Jul 6, 2018

Hackwar commented May 24, 2018 •

edited by infograf768

carlitorweb May 24, 2018 •

edited

carlitorweb commented May 24, 2018 •

edited

carlitorweb Jun 18, 2018 •

edited