[4.0] Fix smart search fatal error

[joomdonation]

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 Smart Search option "Filter Numeric Terms" trigger fatal error message #35539 , confirm the issue
2. Apply patch, confirm the issue fixed.

[chmst]

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

[richard67]

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

[richard67]

RTC

---

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

[brianteeman]

Did you actually check and see if the option works?

[brianteeman]

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.

[chmst]

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.

[richard67]

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

[richard67]

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

[richard67]

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

[richard67]

Back to pending.

---

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

[richard67]

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

[richard67]

RTC

---

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

[brianteeman]

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

[richard67]

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

[richard67]

P.S.: See here https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_finder/src/Indexer/Indexer.php#L937-L940 .

[richard67]

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

[brianteeman]

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

[richard67]

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.

[brianteeman]

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

Never said it was

[richard67]

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

[brianteeman]

As I am failing to explain please test #35542

[richard67]

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.

[brianteeman]

No fatal error with my pr

[richard67]

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.

[brianteeman]

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

[richard67]

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

[brianteeman]

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

[EJBJane]

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

[bembelimen]

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?

[EJBJane]

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

[joomdonation]

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

[joomdonation]

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.

[EJBJane]

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

[bembelimen]

Thx

[brianteeman]

sad face - this is just masking the real problem