TB 1.0.8 seems to break Elasticsearch module

[dynambee]

I'm hosting on Cloudways and have been trying to narrow down this problem.

All of these are fresh installs. Modules updated as requested. SSL enabled. Debugging turned on. Everything else in default settings.

TB 1.0.5 = ES module runs fine

TB 1.0.7 = ES module runs fine

TB 1.0.8 = ES module installs, indexes, but the entire front office ceases to function.
Back office continues to work okay but trying to access the front office results in this error (with debugging turned on):
 

[ThirtyBeesException]
[ PS_CURRENCY_DEFAULT] is not a valid configuration key
at line 1005 in file classes/Configuration.php

1000.      * @since   1.0.8
1001.      */
1002.     protected static function validateKey($key)
1003.     {
1004.         if ( ! Validate::isConfigName($key)) {
1005.             $e = new PrestaShopException(sprintf(
1006.                 Tools::displayError('[%s] is not a valid configuration key'),
1007.                 Tools::htmlentitiesUTF8($key)
1008.             ));
1009.             die($e->displayMessage());
1010.         }

ConfigurationCore::validateKey - [line 367 - classes/Configuration.php] - [1 Arguments]

ConfigurationCore::get - [line 484 - modules/elasticsearch/elasticsearch.php] - [1 Arguments]

Elasticsearch->hookDisplayTop - [line 776 - classes/Hook.php] - [1 Arguments]

HookCore::coreCallHook - [line 484 - classes/Hook.php] - [3 Arguments]

HookCore::execWithoutCache - [line 288 - classes/Hook.php] - [7 Arguments]

HookCore::exec - [line 381 - classes/controller/FrontController.php] - [1 Arguments]

FrontControllerCore->initContent - [line 53 - controllers/front/IndexController.php]

IndexControllerCore->initContent - [line 262 - classes/controller/Controller.php]

ControllerCore->run - [line 253 - classes/controller/FrontController.php]

FrontControllerCore->run - [line 837 - classes/Dispatcher.php]

DispatcherCore->dispatch - [line 33 - index.php]

Since that code chunk starts with "@since 1.0.8" I'm guessing that did something that broke the way the ES module works.

Any chance of a fix for this? I'm more than happy to provide TB devs with all credentials for the test servers I set up to try and figure this out. I'll be offline for a few hours (nearly 4am here, time for sleep) but will of course be back online in 5 or 6 hours.

Thanks greatly for any assistance.


Edit: Forgot to mention my setup:

Cloudways VULTR server, 2 cores, 4GB memory
PHP 7.1
MySQL 5.7
Elasticsearch 5.4.3

Edited June 12, 2019 by dynambee

[datakick]

Apply this fix: https://github.com/thirtybees/elasticsearch/commit/49bfd11fd7e673d547de6115a13972a650c60f31

 

[datakick]

Thanks for this testing, really appreciate it. If you could,  please test against bleeding edge as well. Many 1.0.8 errors are already fixed

[dynambee]

Oh, and if I disable the ES module in the back office the front office runs normally again.

[dynambee]

4 minutes ago, datakick said:

Apply this fix: https://github.com/thirtybees/elasticsearch/commit/49bfd11fd7e673d547de6115a13972a650c60f31

Thank you for the quick reply. Is that something that would have been updated by the Core Updater if run against 1.0.8? If so that fix won't fix the problem because I first had the problem a couple of days ago on 1.0.8 with all the Bleeding Edge updates applied. If that is something that the Bleeding Edge updates don't do then it could easily fix it.

Edit: Ah, I see, that is a fix to the ES module. Okay, I'll try that now. Wonder why it didn't break things on 1.0.7 or before though.

Edited June 12, 2019 by dynambee

[dynambee]

Yup, that fixed it.

Should I be applying any other fixes?

Thanks very much for the help.

[datakick]

 

Quote

Edit: Ah, I see, that is a fix to the ES module. Okay, I'll try that now. Wonder why it didn't break things on 1.0.7 or before though.

Prior 1.0.8, core used to be more benevolent to validation errors in configuration keys. Configuration::get() and Configuration::update() allowed keys that didn't meet validation criteria, like in this case (extra space).

It might look like completely unnecessary check - after all, it worked so far, so why change it? Well, it actually worked only in this particular place. But it caused bugs and problems in other places.

For example, Configuration is a subclass of ObjectModel. So we don't need to use Configuration::get() method, we can also load configuration entries using ObjectModel interface. For this particular entry, if we try to load it from db and immediately save it back, an exception would be thrown. That's beecause `key` field would not pass validation check enforced by ObjectModel.

And that's very, very wrong. No developer would ever expect this

// load configuration entry from database
$config = new Configuration($configId);
// and save it back without any change
$config->save();
// we might never get here because save throwed exception

By being more strict, and enforcing higher quality standards for modules, thirtybees is preventing these kind of weird bugs. Unfortunately, sometimes it's necessary to patch modules. But that's all right, after all, they contain bug. 

[dynambee]

10 hours ago, datakick said:

By being more strict, and enforcing higher quality standards for modules, thirtybees is preventing these kind of weird bugs. Unfortunately, sometimes it's necessary to patch modules. But that's all right, after all, they contain bug. 

I'm very much in favor of stricter code standards, you won't hear any complaints from me!

I may have discovered another problem with the module, just trying to narrow it down a bit now. The indexing seems to work only once if any settings are changed in the module. Will try to replicate with earlier versions of TB and see if I can figure out what is going on.

[dynambee]

22 hours ago, datakick said:

Thanks for this testing, really appreciate it. If you could,  please test against bleeding edge as well. Many 1.0.8 errors are already fixed

Edit: See update in following message.

I finally have a proper test platform set up where I can recover the original base install & db in a matter of a single click and a few seconds. Saves a lot of time.

Anyway, I have found another problem--or at least I think it is a problem. This is on a fresh install of 1.0.8 bleeding edge (all core updates done). Besides the core updater only elasticsearch has been added.

System specs & service versions are the same as in my first comment above.

For this test I will use the search term honey. The honey product is in the Tea category. (Other products have the same results as best I can tell.)

If I use the elasticsearch search box to search for honey from the top page of the website, everything works as expected and only the one honey product is returned.

However if I first click on a category ("Coffee and Tea" or "Tea") and then search for honey every product in that category is returned. If I search for honey in the Coffee category then all the products in the Coffee category are returned, which of course does not include the honey product which is in the Tea category.

In all cases just above the returned products it says "Showing 1 - 1 of 1 item" regardless of how many items are returned.

I would expect that when on the top page, in the "Coffee and Tea" area and in the "Tea" area that searching for honey would return just the one honey product. In the "Coffee" area I would expect there to be zero search results.

Ideally if there are results elsewhere on the site (outside the selected category) it would be fantastic if the site gave a message about that so that users would realize that the item they searched for does exist on the site, just not in the category they are in. Something like what Amazon does.

Alternatively the currently displayed category could be ignored completely and then categories could be a post-search filter.

As always I would be happy to give you access to my test server if it would be useful.

Edited June 14, 2019 by dynambee

[dynambee]

Hmmmmm, actually, it seems that if someone actually clicks on the little magnifying glass search button for the elasticsearch search field that the results are okay.

It's the "instant search" as-you-type results that are screwy. Pressing enter (totally standard thing to do IMO) it does not trigger the search button and the "instant search" results stay as the incorrect results.

So the actual bug would seem to be that the "instant search" results and the actual "click on the search button" results are different. Hopefully this is easier to fix.

Edit: This problem exists in varying degrees in 1.0.5, 1.0.7, and non-bleeding-edge 1.0.8 as well. I have not tested other versions at this time.

Edited June 14, 2019 by dynambee

[musicmaster]

On 6/12/2019 at 9:35 PM, datakick said:

 

Prior 1.0.8, core used to be more benevolent to validation errors in configuration keys. Configuration::get() and Configuration::update() allowed keys that didn't meet validation criteria, like in this case (extra space).

Having seen more extra space trouble (with the Mollie module) I wonder whether this might be a bit too radical. Did you consider alternatives such as warnings instead of errors or having an error message that explicitly tells you that there is an extra space problem? The latter might be a check for a space at the start or the end of the string after the error has been discovered and an extra message "It looks like you added a space to the key." behind the default message.