Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make "-" searchable - backport #1668

Closed
simonrl opened this issue Jan 17, 2020 · 5 comments
Closed

Make "-" searchable - backport #1668

simonrl opened this issue Jan 17, 2020 · 5 comments
Assignees
Labels

Comments

@simonrl
Copy link
Contributor

simonrl commented Jan 17, 2020

In reference to this issue: #1011

Any idea how I implement only the changes from #1022 without updating to the latest version?

I tried defining the analyzers with new params in my own elasticsuite_analysis.xml file in a new module, but get the error message that multiple nodes have been found for /analysis/analyzers/analyzer[@name='standard' and @language='default']/filters/filter

If overriding analyzer definitions is not possible, would you merge a backport to the 2.3.x branch if I create one, even if this version is no longer supported?

Thanks
Simon

@rbayet

@rbayet rbayet self-assigned this Jan 20, 2020
@rbayet
Copy link
Collaborator

rbayet commented Feb 24, 2020

Hello @simonrl,

Sorry I'm only coming back to this subject.
Re-reading with fresh eyes your problem :

I tried defining the analyzers with new params in my own elasticsuite_analysis.xml file in a new module, but get the error message that multiple nodes have been found for /analysis/analyzers/analyzer[@name='standard' and @language='default']/filters/filter

I'm realising that what you're missing is probably just the piece of code which merges different elasticsuite_analysis.xml files.
Could your provide me with your custom elasticsuite_analysis.xml section ?
I'll try to see if it I'm correct.

Regards,

@rbayet rbayet assigned simonrl and unassigned rbayet Feb 24, 2020
@simonrl
Copy link
Contributor Author

simonrl commented Mar 4, 2020

Hey @rbayet

sorry for the delayed reply.

I basically just copied src/module-elasticsuite-core/etc/elasticsuite_analysis.xml to my custom module and re-defined the analyzers in there, hoping they would be preferred over the core-analyzers. (I also added a <depends> node in my module.xml).

<?xml version="1.0"?>
<analysis xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:module:Smile_ElasticsuiteCore:etc/elasticsuite_analysis.xsd">
<analyzers>
        <analyzer name="standard" tokenizer="whitespace" language="default">
            <filters>
                <filter ref="lowercase" />
                <filter ref="ascii_folding" />
                <filter ref="trim" />
                <filter ref="elision" />
                <filter ref="word_delimiter" />
                <filter ref="standard" />
            </filters>
            <char_filters>
                <char_filter ref="html_strip" />
            </char_filters>
        </analyzer>
        <!-- more analyzers I changed -->
</analyzers>
</analysis>

I assume this is not correct?
Also, if the ability to merge analyzer-files was added after Elasticsuite 2.3, this may be my problem. We're currently updating and if the old version is the problem, then feel free to close this issue.

Thanks
Simon

@rbayet
Copy link
Collaborator

rbayet commented Mar 6, 2020

Hello @simonrl,

Actually, I cannot do what you want to achieve in a 2.3.4 either.
That is to say :

  • redefine <analyzer name="standard" [...] language="default">
  • with the same or a different tokenizer ...

I'm not sure this is totally a merge issue on the /analysis/analyzers/analyzer.
See \Smile\ElasticsuiteCore\Index\Analysis\Config\Reader::$_idAttributes :

    /**
     * List of attributes by XPath used as ids during the file merge process.
     *
     * @var array
     */
    protected $_idAttributes = [
        '/analysis/char_filters/char_filter' => ['name', 'language'],
        '/analysis/filters/filter'           => ['name', 'language'],
        '/analysis/analyzers/analyzer'       => ['name', 'language'],
    ];

Logically, all the analyzer nodes with the same name and language should be merged, whether or not you change the value for the tokenizer attribute.

Actually, what I had managed was to completely redefine analyzer name="standard" but

  • not changing the tokenizer
  • and doing it for a specific language identifier and not for "default"

Intuitively, the issue should be somewhere in \Smile\ElasticsuiteCore\Index\Analysis\Config\Converter::getDefaultConfiguration, but the reported exception is located in \Magento\Framework\Config\Dom.

I'll try to follow the rabbit down that hole, but meanwhile, you might have one option : making your changes on the analyzer name="standard" on anything BUT language="default".
That might be a bit cumbersome if you support many languages in your project(s).

Regards,

@rbayet rbayet assigned simonrl and unassigned rbayet Mar 6, 2020
@simonrl
Copy link
Contributor Author

simonrl commented Mar 9, 2020

Hi @rbayet

thanks for the explanation.

If you consider this a real issue, you can of course try to solve it. We don't need it though, so if my request is the sole reason for you to check this, don't waste your time there :) Our use-case was to implement changes from future versions in Elasticsuite 2.3, and as we're currently upgrading, that's no longer relevant.

Thanks
Simon

@no-response no-response bot removed the needs update label Mar 9, 2020
@romainruaud romainruaud assigned rbayet and unassigned simonrl Aug 4, 2020
@romainruaud
Copy link
Collaborator

I close, this topic has been stalled for too long, I'm not even sure that's still accurate.

There might be a remaining topic on XML files merging for analyzer that remains open in #2194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants