Skip to content

Conversation

@cammonro
Copy link
Contributor

Summary

The PR provides support for extension via the search adapter:

  • Refactored IndexOptionsBuilder with improved error handling
  • Algolia logger now accepts context for debugging data
  • Support added for overrides of timeout configuration in AlgoliaConnector
  • Fixed integration test and added unit tests for index options
  • The unused legacy class AlgoliaHelper has been removed
  • Added comment model link utilities via abstract class for config admin notices (maintains selected scope)
  • Monolog v2 deprecation observed but maintaining compatibility with Magento 2.4.7 until v2.4.9 release

Result

Updated logging:
image

Fixed integration test:
image

IndexOptionsBuilder unit tests:
image

Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Good job! 👏

I have a few comments, let me know what you think about it :)

*
* @throws NoSuchEntityException
*/
protected function safeBuildWithComputedIndex(string $indexSuffix, int $storeId, bool $isTmp = false): IndexOptionsInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

If there a particular reason not to simply replace buildWithComputedIndex by this implementation on the parent class ?
The Index Options management begins to be quite complex and I'm wondering if it's necessary

'isTmp' => $indexOptions->isTemporaryIndex()
]
);
throw new AlgoliaException($msg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Which function should we use to please our Codacy friend here ? addslashes() maybe ?

use Algolia\AlgoliaSearch\Service\AlgoliaConnector;
use Magento\Framework\Exception\NoSuchEntityException;

class BackendSearch
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we plan to re-use the getSearchResult() method for the search adapter ?
Currently its only used in the AdapterHelper::getDocumentsFromAlgolia() method, which is not supposed to be used as well. (also in an integration test though but we can change it I guess)
If we don't plan to re-use them, maybe this is the right time to remove them ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants