? ? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
12 Sep 2021

Pull Request for Issue #35539.

Summary of Changes

This PR adds a check to only execute the insert query if there is actual token to insert to prevent SQL error during index process.

Testing Instructions

  1. Follow #35539 , confirm the issue
  2. Apply patch, confirm the issue fixed.
avatar joomdonation joomdonation - open - 12 Sep 2021
avatar joomdonation joomdonation - change - 12 Sep 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 12 Sep 2021
Category Administration com_finder
avatar joomdonation joomdonation - change - 12 Sep 2021
Labels Added: ?
avatar chmst chmst - test_item - 12 Sep 2021 - Tested successfully
avatar chmst
chmst - comment - 12 Sep 2021

I have tested this item successfully on 4290b90

I have tested this PR and it resolves the problem.

The articles stored while the error occurred are not displayed in articles overview. It is not related to this PR.


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

avatar richard67 richard67 - test_item - 12 Sep 2021 - Tested successfully
avatar richard67
richard67 - comment - 12 Sep 2021

I have tested this item successfully on 4290b90


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

avatar richard67 richard67 - change - 12 Sep 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 12 Sep 2021

RTC


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

avatar brianteeman
brianteeman - comment - 12 Sep 2021

Did you actually check and see if the option works?

avatar brianteeman
brianteeman - comment - 12 Sep 2021

AFAICT the only thing you can have tested is that there is no error when this option is enabled, you are not actually testing if the option works. From what I can tell this option can not work as it hasnt been implemented. So it is not possible to say that the problem is fixed.

avatar chmst
chmst - comment - 12 Sep 2021

This is correct @brianteeman - it is a quick solution which fixes the fatal error and storing false duplicates in content. It surely needs more investigation.

avatar richard67
richard67 - comment - 12 Sep 2021

@brianteeman I don't see that this functionality is not implemented at all. At least it is implemented in the same way as the common search terms, as far as I can see in code.

It is e.g. read here https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_finder/src/Indexer/Indexer.php#L914 and then checked here https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_finder/src/Indexer/Indexer.php#L942-L945 .

The "numeric" property of the token is set as well as the common "property" by the tokenizer. If the site is multilingual, the tokenizer of the translation is used if available:

https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_finder/src/Indexer/Helper.php#L113

For testing that it would need a translation which provides that, same as with testing the common search terms (if not English).

At least that's what I can find in code. Of course I might be wrong. Maybe @Hackwar can shed a light on it.

avatar richard67
richard67 - comment - 12 Sep 2021

Hmm, @brianteeman could be right and it's not fully implemented because in database tables for terms and tokens have a column "common" but don't have a column "numeric".

avatar richard67 richard67 - test_item - 12 Sep 2021 - Not tested
avatar richard67
richard67 - comment - 12 Sep 2021

I have not tested this item.

Reverting my test now as I'm unsure either now if this is the right fix. If the new article doesn't contain any numeric words, I would not expect anything to be filtered out and so there should be tokens found, like it is with the common search words if those aren't given for some language.


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

avatar richard67 richard67 - change - 12 Sep 2021
Status Ready to Commit Pending
avatar richard67
richard67 - comment - 12 Sep 2021

Back to pending.


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

avatar richard67 richard67 - test_item - 12 Sep 2021 - Tested successfully
avatar richard67
richard67 - comment - 12 Sep 2021

I have tested this item successfully on 4290b90

Thinking it over and reviewing the code, I think this PR is right, and it fixes the issue.

The two "continue" statements here may result in the "values" property of the query not being set, and so it needs to check for that before executing the query.

Another question is why the "values" property of the query is not set when the option to filter numerics is on. This is a separate issue which doesn't change anything on the need for the check added by this PR.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35540.
avatar richard67 richard67 - change - 12 Sep 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 12 Sep 2021

RTC


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

avatar brianteeman
brianteeman - comment - 12 Sep 2021

This bug could also be fixed by removing the option. As the option doesn't do anything (it can't) then that would be a more sensible fix than having an option that doesnt do anything. Merging this PR will just mask the problem that the option doesn't do anything

avatar richard67
richard67 - comment - 12 Sep 2021

@brianteeman When the option will be removed then it still will need the fix from this PR because due to the other filtering for common search terms the values property of the query still might be not set.

avatar richard67
richard67 - comment - 12 Sep 2021

@brianteeman You can test what I said as follows: Have a multilingual site with English language and others. Switch off the "filter numeric terms" option, if on, and switch on the "filer common words" option. Then create an article, assign it to English language and make sure that the article content consists only of words from this list here: https://github.com/joomla/joomla-cms/blob/4.0-dev/language/en-GB/com_finder.commonwords.txt . Then try to save. Result: Same error as mentioned in the issue. Apply the patch again and try again. Result: Works. Note that this only works when an article is assigned to the English language and not to language all, and that requires a multilingual site.

avatar brianteeman
brianteeman - comment - 12 Sep 2021

@brianteeman When the option will be removed then it still will need the fix from this PR because due to the other filtering for common search terms the values property of the query still might be not set.

I meant completely remove everything associated with this broken option. Not just the xml

@brianteeman You can test what I said as follows: Have a multilingual site with English language and others. Switch off the "filter numeric terms" option, if on, and switch on the "filer common words" option. Then create an article, assign it to English language and make sure that the article content consists only of words from this list here: https://github.com/joomla/joomla-cms/blob/4.0-dev/language/en-GB/com_finder.commonwords.txt .

Of course. But it would also be resolved by removing the broken code.

avatar richard67
richard67 - comment - 12 Sep 2021

Of course. But it would also be resolved by removing the broken code.

@brianteeman The code for the "Filter common words" is not broken, and as I clearly pointed out this one also requires the fix from this PR here, even if you completely remove the other option "Filter numeric terms" with all its code.

avatar brianteeman
brianteeman - comment - 12 Sep 2021

@brianteeman The code for the "Filter common words" is not broken,

Never said it was

avatar richard67
richard67 - comment - 12 Sep 2021

@brianteeman The code for the "Filter common words" is not broken,

Never said it was

@brianteeman The do the test which I have described in my comment and you will see that the fix provided by this PR here is not only needed for the broken "Filter numeric terms" but also for the not broken "Filter common words" option.

avatar brianteeman
brianteeman - comment - 12 Sep 2021

As I am failing to explain please test #35542

avatar richard67
richard67 - comment - 12 Sep 2021

As I am failing to explain please test #35542

And as I am failing to explain please test what I have described in my comment, regardless of that option. You will get the fatal error without this PR and you will also get it with your PR and you will not get it with this PR.

avatar brianteeman
brianteeman - comment - 12 Sep 2021

No fatal error with my pr

avatar richard67
richard67 - comment - 12 Sep 2021

No fatal error with my pr

@brianteeman Also not when you do exactly as I described here;

Have a multilingual site with English language and others.

Switch off the "filter numeric terms" option, if on, and switch on the "filer common words" option.

Then create an article, assign it to English language and make sure that the article content consists only of words from this list here: https://github.com/joomla/joomla-cms/blob/4.0-dev/language/en-GB/com_finder.commonwords.txt .

Then try to save. Result: Same error as mentioned in the issue.

Apply the patch again and try again.

Result: Works.

Note that this only works when an article is assigned to the English language and not to language all, and that requires a multilingual site.

avatar brianteeman
brianteeman - comment - 12 Sep 2021

Note that this only works when an article is assigned to the English language and not to language all, and that requires a multilingual site.

I missed that part

avatar richard67
richard67 - comment - 12 Sep 2021

@brianteeman Don't get me wrong please. If the option is broken and it makes sense to remove it, then let's do it and I will support your draft PR. I only think that the fix of this PR here still will be necessary when the option has been removed, that's what I try to explain (and what I've tested).

avatar brianteeman
brianteeman - comment - 12 Sep 2021

I updated my pr to include this fix.

@richard67 under a previous maintainer it was common for this type of quick fix to be applied without addressing the full problem. This always resulted in issues further down the line with band-aids on top of band-aids. Now that they are no longer a maintainer I hope we can stop applying band-aids and look at the whole problem

avatar EJBJane
EJBJane - comment - 17 Sep 2021

I have this problem but the new code actually doesnt fix it.

avatar bembelimen
bembelimen - comment - 22 Sep 2021

I have this problem but the new code actually doesnt fix it.

@EJBJane could you please describe what you did and what failed, so we can fix it?

avatar EJBJane
EJBJane - comment - 23 Sep 2021

Hi there,
Okay so this is Joomla! version is "‎4.0.3. I am running this on a development area/url on my webserver, if need can give you access. I added two screenshots. FYI I have over 2000 articles, 100 categories and 50 tags. When I turn ON Filter Numeric Terms proceed to indexing, an error appears, as shown in screenshot. When I turn it OFF the indexing proceeds as normal.
For now I have disabled it. Does this help?
Liz
Error.pdf
Enabled_numeric.pdf

avatar joomdonation
joomdonation - comment - 23 Sep 2021

@EJBJane It is strange that you still get SQL error with the new code. You can send an email to tuanpn@joomdonation.com with the access, I will be happy to take a look at the error to see why it still happens with that new code.

avatar joomdonation
joomdonation - comment - 23 Sep 2021

OK. So I got access to the site:

  • Checked and the code from PR was not updated to that site
  • I update the code from this PR to the site, then run the index and it worked well. All data were indexed and no error

So I can only guess that @EJBJane has not added code from this PR to her site yet.

avatar EJBJane
EJBJane - comment - 23 Sep 2021

It works now, with the kind help of Tuan. It was difficult for me to know which was the right file to replace and which code. I am very new to Github (I never really needed it).

avatar bembelimen bembelimen - change - 27 Sep 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-09-27 11:02:53
Closed_By bembelimen
Labels Added: ?
avatar bembelimen bembelimen - close - 27 Sep 2021
avatar bembelimen bembelimen - merge - 27 Sep 2021
avatar bembelimen
bembelimen - comment - 27 Sep 2021

Thx

avatar brianteeman
brianteeman - comment - 27 Sep 2021

sad face - this is just masking the real problem

Add a Comment

Login with GitHub to post a comment