Joomla! Issue Tracker | Joomla! CMS #12253

Fixed in Code Base
21 Aug 2018
Medium
Build: staging
# 12253
Diff
frankmayer:Indexer

Failure

Success Hound No violations found. Woof! Details
Success continuous-integration/travis-ci/pr The Travis CI build passed Details
Success continuous-integration/drone/pr the build was successful Details
Failure continuous-integration/appveyor/pr AppVeyor build failed Details
Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details

User tests: Successful: Unsuccessful:

frankmayer
2 Oct 2016

[UPDATED (8.1.2017) - see third paragraph]
Some optimization and cleanup work on the indexer part, which might improve its performance a bit.

Summary of Changes

I removed some senseless parts of the fr.php getStem() method (given that this part did work correctly in this state). The explanation is that the condition if (($matches[2] != '*') || ($intact)) was always true because of $intact being always true. So this part was removed. The associated else part was removed for the same reason.
fixed a bug in query.php on lines 197 & 207 where previously checks were performed on the wrong options.
replaced some count($var) which were essentially checking if the array is not empty and some !empty($var) methods with a faster alternative which is (bool) $var.
While for count() this is up to 2.5 times faster (according to my own benchmarks), there is only about a 10% performance gain when replacing !empty() (Which basically seems to be the inversion of !empty(), see here).

The rest is some cleanup and optimizations here and there:

Replace deprecated JString
Fix some ambiguous PREG parameters
Type safe comparisons
docblock fixes
Removed some unnecessary conditions
Simplifications of nested positive ifs and ternary operators to Elvis
Added a few fields to token in order to not adding them dynamically, which is more expensive

[Update 8.1.2017]
Since this PR, I have also submitted a few others relating to the indexer. These should best be merged before this one. After that, I can resolve the conflicts, here.

Work on com_finder (both admin and site) #12254
[Indexer] Replace StringHelper::trim() calls with simple trim() #13478
Optimized FinderIndexerHelper->stem() method #13480
Cleanup and optimization in FinderIndexerDrivers #13511

Testing Instructions

Code review should suffice

a9fbfec 1 Oct 2016

Merge some unset()

8e87b73 2 Oct 2016

Lots of work on the indexer itself:

848e7d8 2 Oct 2016

Some Elvis's and a missing @since annotation

d4b5526 2 Oct 2016

One more StringHelper...

71054f8 2 Oct 2016

Codestyle...

f3625aa 2 Oct 2016

Not to potentially break BC, reversed back to `!empty($var)` operation but with the non inverted style `(bool) $var`

327a1bc 2 Oct 2016

Formatting

frankmayer - open - 2 Oct 2016

frankmayer - change - 2 Oct 2016

Status

New

⇒

Pending

joomla-cms-bot - change - 2 Oct 2016

Labels

Added: ?

joomla-cms-bot - change - 2 Oct 2016

Category

Administration Components CLI

⇒

Administration com_finder CLI Components

8b0a250 13 Dec 2016

Removed repeated `use`

5f3cd39 13 Dec 2016

Some extra work according to @andrepereiradasilva's notes

andrepereiradasilva - comment - 13 Dec 2016

I have tested this item ✅ successfully on 5f3cd39

code review

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

andrepereiradasilva - test_item - 13 Dec 2016 - Tested successfully

frankmayer - comment - 26 Dec 2016

I wonder if this one could get pushed to the top, as it provides a few optimizations, that especially in this case could make a difference in performance, because of the many loops and comparisons, etc.

3145c15 6 Jan 2017

Merge branch 'staging' into Indexer

frankmayer - edited - 6 Jan 2017

frankmayer - change - 8 Jan 2017

The description was changed

frankmayer - edited - 8 Jan 2017

frankmayer - change - 8 Jan 2017

The description was changed

frankmayer - edited - 8 Jan 2017

frankmayer - change - 8 Jan 2017

The description was changed

frankmayer - edited - 8 Jan 2017

frankmayer - change - 5 Feb 2017

The description was changed

frankmayer - edited - 5 Feb 2017

0edf998 27 May 2017

Merge branch 'staging' into Indexer

rdeutz - change - 27 May 2017

The description was changed

1bf4190 1 Jun 2017

Merge branch 'staging' into Indexer

2ca0681 1 Jun 2017

Additional changes according to reviewer's comments

10ed679 1 Jun 2017

Merge remote-tracking branch 'origin/Indexer' into Indexer

11b1789 12 Jun 2017

Merge branch 'staging' into Indexer

5e1cbdd 13 Jun 2017

Merge branch 'staging' into Indexer

frankmayer - change - 28 Jun 2017

The description was changed

frankmayer - edited - 28 Jun 2017

frankmayer - change - 25 Jul 2017

The description was changed

frankmayer - edited - 25 Jul 2017

frankmayer - change - 1 Aug 2017

The description was changed

frankmayer - edited - 1 Aug 2017

frankmayer - comment - 1 Aug 2017

@andrepereiradasilva & @Quy could you guys give a green light for this PR? You have already done the code review in the past and there were only minor changes to this PR as @Quy proposed. Thanks guys.

frankmayer - comment - 4 Aug 2017

Bump @wilsonge would be nice if we can also get this in? @andrepereiradasilva had tested it and @Quy had a few suggestions, which I adapted afterwards. Essentially we have one successful test at the moment, with a few minor changes afterwards.

4e4fd10 24 Aug 2017

Merge branch 'staging' into Indexer

frankmayer - comment - 24 Aug 2017

@wilsonge, @mbabker any chance this one can make it into 3.8 or are we too far along?
It was reviewed and deemed ok, prior to some resolving of conflicts.

0944ec9 25 Aug 2017

Code style

frankmayer - comment - 25 Aug 2017

@mbabker BTW, I do have some more work on type-safe comparisons, which I have split up into 7 branches. They only need code review. Shall I make those PR's now for inclusion into 3.8.0 or later for 3.8.1 or 3.9?

frankmayer - comment - 8 Nov 2017

@mbabker Bump for 3.9?

frankmayer - comment - 13 Mar 2018

@mbabker bump? This PR is one and a half years old, now... (Remark: This was tested successfully by one peer before doing some minor proposed changes/conflict resolutions)

Quy - comment - 13 Mar 2018

To reproduce:

Click Clear Index button.
Click Index button.

In the PHP error log:

PHP Notice:  Trying to get property 'term' of non-object in \administrator\components\com_finder\helpers\indexer\indexer.php on line 523
PHP Notice:  Trying to get property 'stem' of non-object in \administrator\components\com_finder\helpers\indexer\indexer.php on line 524
PHP Notice:  Trying to get property 'common' of non-object in \administrator\components\com_finder\helpers\indexer\indexer.php on line 525
PHP Notice:  Trying to get property 'phrase' of non-object in \administrator\components\com_finder\helpers\indexer\indexer.php on line 526
PHP Notice:  Trying to get property 'weight' of non-object in \administrator\components\com_finder\helpers\indexer\indexer.php on line 527
PHP Notice:  Trying to get property 'language' of non-object in \administrator\components\com_finder\helpers\indexer\indexer.php on line 529

frankmayer - comment - 14 Mar 2018

@Quy is this the result of you testing this PR again? And those notices were not there prior to the patch?

Quy - comment - 14 Mar 2018

I may have only did code review previously. Yes these notices generated with this PR.

frankmayer - comment - 14 Mar 2018

OK, thanks, I'll take a look.

8220232 17 May 2018

Merge branch 'staging' into Indexer

roland-d - comment - 22 Jul 2018

@frankmayer Can you have a look at the notices?

Tchangue - test_item - 23 Jul 2018 - Tested successfully

Tchangue - comment - 23 Jul 2018

I have tested this item ✅ successfully on 0944ec9

Reviewed code. Everything fine.
@icampus

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

durubayram - test_item - 23 Jul 2018 - Tested successfully

durubayram - comment - 23 Jul 2018

I have tested this item ✅ successfully on 0944ec9

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

franz-wohlkoenig - change - 23 Jul 2018

Status

Pending

⇒

Ready to Commit

franz-wohlkoenig - comment - 23 Jul 2018

Ready to Commit after two successful tests.

brianteeman - comment - 23 Jul 2018

please remove RTC until the reported php notices are resolved #12253 (comment)

franz-wohlkoenig - change - 23 Jul 2018

Status

Ready to Commit

⇒

Pending

franz-wohlkoenig - comment - 23 Jul 2018

reset Status on "Pending" as stated above.

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

roland-d - comment - 24 Jul 2018

@frankmayer Can you please address the mentioned issue so we can move it forward?

frankmayer - comment - 7 Aug 2018

Sorry for the delay people. Had lots of other stuff on my plate. Will try to resolve this ASAP. Thanks

f172764 7 Aug 2018

Merge branch 'staging' into Indexer

5e74e91 7 Aug 2018

Fix misbehavior due to not correctly handling the single object case of mixed variable (array|object)

frankmayer - comment - 7 Aug 2018

Should be OK now. Thanks for testing and reporting the issue.

6d2c4f9 7 Aug 2018

Pass float value to db->escape as float may produce problems for some locales.

8125d35 7 Aug 2018

"Make values float again" - (pun intended)

frankmayer - comment - 7 Aug 2018

I think the drone failure is false... can someone restart that or will I have to push again?

roland-d - comment - 7 Aug 2018

@frankmayer The drone failure is because of this:

07 08 2018 16:58:13.155:ERROR [Firefox 59.0.0 (Ubuntu 0.0.0)]: TypeError: a.ui is undefined
at http://localhost:9876/base/media/jui/js/jquery.ui.sortable.min.js?3ebfa0c205cdb3b90e6858aea5c05607b57e93c8:17

mbabker - comment - 7 Aug 2018

It's the same constant false positive. Don't worry about it.

rdeutz - comment - 7 Aug 2018

in this case we had two different wrongly failed test :-) all good now.

roland-d - test_item - 7 Aug 2018 - Tested successfully

roland-d - comment - 7 Aug 2018

I have tested this item ✅ successfully on 8125d35

After applying the PR I have done the following tests:

Run the indexer, indexing goes well
Search different terms and ways on the frontend
Added a search filter
Menu item using a search filter

All without problems.

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

frankmayer - comment - 7 Aug 2018

Ah, great :) Thanks guys. And thank you @roland-d for testing ?

frankmayer - comment - 7 Aug 2018

@Quy can you make the second human test in order to finally give this one a proper send-off? ;)

Quy - test_item - 8 Aug 2018 - Tested successfully

Quy - comment - 8 Aug 2018

I have tested this item ✅ successfully on 8125d35

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

Quy - change - 8 Aug 2018

Status

Pending

⇒

Ready to Commit

Quy - comment - 8 Aug 2018

RTC

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

mbabker - change - 21 Aug 2018

Status	Ready to Commit	⇒	Fixed in Code Base
Closed_Date	0000-00-00 00:00:00	⇒	2018-08-21 03:57:01
Closed_By		⇒	mbabker
Labels	Added: ?

mbabker - close - 21 Aug 2018

mbabker - merge - 21 Aug 2018

dagalumin - comment - 9 Oct 2018

This working on indexer released with 3.8.12 has introduced some issues in saving and indexing large articles. Please see issue #22098 and #22220

Add a Comment

Older
Newer

Joomla! Issue Tracker - CMS

[#12253] - Work on indexer

Summary of Changes

Testing Instructions

Add a Comment