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

Unable to override Fulltext\Collection in module et/di.xml #7734

Closed
joachimVT opened this issue Dec 9, 2016 · 29 comments
Closed

Unable to override Fulltext\Collection in module et/di.xml #7734

joachimVT opened this issue Dec 9, 2016 · 29 comments
Labels
bug report Component: Search Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@joachimVT
Copy link

joachimVT commented Dec 9, 2016

Preconditions

  1. Magento 2.1.1
  2. Using Ubuntu/trusty64 vagrant-box, php7, apache

Steps to reproduce

  1. Add a new custom module
  2. Update etc/di.xml in your custom module
<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../../../lib/internal/Magento/Framework/ObjectManager/etc/config.xsd">
    <preference for="Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection" type="Sanimarkt\App\Model\ResourceModel\Fulltext\Collection" />
</config>

  1. Add new Collection.php file and override _renderFiltersBefore() method
<?php

namespace Sanimarkt\App\Model\ResourceModel\Fulltext;

/**
 * Fulltext Collection
 * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
 */
class Collection extends \Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection
{
 protected function _renderFiltersBefore()
 {
  die('stopping here');
 }
}
  1. clear cache
  2. Make a search request

Expected result

  1. The search request should stop and show 'stopping here' on the screen

Actual result

  1. Nothing happens the custom class Sanimarkt\App\Model\ResourceModel\Fulltext\Collection.php is not executed.
@igrybkov
Copy link
Contributor

igrybkov commented Feb 3, 2017

Hi,

Thanks for reporting this issue.

We've created internal ticket MAGETWO-64162 to address this issue.

The reason why it doesn’t work now is because the class \Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection is used in the CatalogSearch with customization through virtual types and there is no direct usage of this class.
Customization of the source class for a virtual type is not possible because the ObjectManager resolves preference before resolving is a virtual type used and if that, then which class it should instantiate.
However, with the current implementation, you can customize specific virtual types and it should help you.

In Magento 2.1.3. I can find only one virtual type for the class Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection:

<virtualType name="Magento\CatalogSearch\Model\ResourceModel\Fulltext\SearchCollection" type="Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection">
    <arguments>
        <argument name="searchRequestName" xsi:type="string">quick_search_container</argument>
    </arguments>
</virtualType>

So to resolve your problem you may define preference for the virtual type:

<preference for="Magento\CatalogSearch\Model\ResourceModel\Fulltext\SearchCollection" type="Sanimarkt\App\Model\ResourceModel\Fulltext\Collection" />

@igrybkov igrybkov added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Feb 3, 2017
@igrybkov igrybkov removed their assignment Feb 3, 2017
@maghamed
Copy link
Contributor

maghamed commented Feb 4, 2017

Hi @joachimVT ,

Could you please provide a bit more details why do you need to extend search Collection?
I am asking because extending of this specific collection is not a good idea (actually Magento discourages inheritance for all classes and entities in general in favor of composition and customization with a help of Dependency Injection/Plugins/Event Observers etc., but especially Search collection is very fragile and overwriting some of the logic inside could lead to broken search functionality).

This happens because of the Search Collection design.
\Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection plays a role of CatalogSearch API.
It uses another Search API (API of module Search - \Magento\Search\Api\SearchInterface) inside (in the method - _renderFiltersBefore). So, after _renderFiltersBefore executed we already have search results and Layered Navigation built. So, for example, applying additional filters to the Select object after that stage will break Layered navigation values.
Overwritten of its logic also a bad idea, because guaranteeing of consistency that total results count and Layered Navigation count are equal becomes your responsibility.

That's why it's better to leave \Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection as is, and customize its behavior with other mechanism. We could advise you how to better solve your specific task if you will shed some light and provide more details.

@sergei-sss
Copy link

sergei-sss commented Mar 27, 2017

Customization of the source class for a virtual type is not possible because the ObjectManager resolves preference before resolving is a virtual type used and if that, then which class it should instantiate.

I don't understand, It is bug or feature? )
Here KAndy wrote, that is a bug and it worked before developer beta.

@igrybkov
Copy link
Contributor

In the comment you mentioned, @kandy talking about an ability to use plugins with virtual types and that's another issue. I'm not sure is it fixed or not, but that's not the same issue as this one.
In this issue, we're discussing the case when the developer wants to override the class from which virtual types are built.
Actually, I've got an update regarding this case. We've decided that this is not an issue and this restriction to rewrite source class of a virtual type is totally correct, and this behavior should not be changed. That's true because in another way it's too easy to break the code which relied on that behavior through a virtual type.
So, the right way to do such customizations is to set the preference for each specific virtual type.

@korostii
Copy link
Contributor

@igrybkov, great comment, thank you for the clarification!

So, the right way to do such customizations is to set the preference for each specific virtual type.

Now that's something that should be put into the devdocs for good.

@igrybkov
Copy link
Contributor

@korostii, probably it should be documented as an answer to the question "how to override virtual type" or even "what to do when I want to override a class which is used as virtual type", but in general, as @maghamed said, Magento discouraging inheritance, and for each specific case it's better to find the way how to customize behavior using plugins and specific API/SPI or another extension points.
For example, the initial case for this issue is an example when developer chose inheritance instead of existing customization point through the Search API. I'm not sure what actually @joachimVT tried to customize, but there should be used another extension point (something behind Search) - which one depends on the specific customization needs. Anyway, if it's something impossible to customize through existing extension points (Search API in this case), then the issue should be raised regarding this case and inheritance may be used as a workaround until an issue will be fixed. But it's never the preferred way to solve the problem.

@joachimVT
Copy link
Author

joachimVT commented Mar 29, 2017

@igrybkov The reason I want to customize the search engine is to use Logical AND search I know this is possible by using the SEARCH REST API (http://devdocs.magento.com/guides/v2.0/howdoi/webapi/search-criteria.html#logical-and-search)

But I wanted this to be possible by searching in the standard search form.

@m2developer
Copy link

@igrybkov Thanks for the explanation.

Right now I have same requirement (I mean to override search collection, with your comment I can able to override the class), But can;t get any search result.

Before overriding I am getting the search result and after overriding I am not getting any search result. Even I tried to search with exact product name but couldn't get any search result. Any comment for this ?

Note : I done the di;compile and cache:flush after changes in di.xml.

@igrybkov
Copy link
Contributor

igrybkov commented Apr 4, 2017

@m2developer, it depends on what you changed. Just overriding with inherited class will not affect the behavior of the system in the way you mentioned.
So, it looks like your code contains some bug.

I cannot assist you regarding your case here (and the same regarding @joachimVT's case) as GitHub is the place for bug reports, but this case is not a bug, and therefore closed. I'll suggest you ask your question with specific code example in the Community Forums or the Magento Stack Exchange, and post the link here for someone who will look for similar question through GitHub.

@m2developer
Copy link

m2developer commented Apr 4, 2017

@igrybkov Thanks for quick response.

For this I just written the same thing which you have mention, and I don't have any external plugins.

Also, I am using Magento 2 EE edition, and I found that module CatalogStaging override this class like

<preference for="Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection" type="Magento\CatalogStaging\Model\ResourceModel\Fulltext\Collection" />

They are not using overriding the class by

Magento\CatalogSearch\Model\ResourceModel\Fulltext\SearchCollection

Any comment on this ?

@m2developer
Copy link

@igrybkov Any comment on this ?

@leedave
Copy link

leedave commented Oct 4, 2017

This is clearly references to the Bugs #9066 and #5590 if not many other tickets here.
A search feature that cannot filter by relevance or change the wording operator from OR to AND is a serious Problem for an online shop. I'm searching for a temporary workaround myself, but can't find any legit method of doing this

-Plugins don't work because of a protected method
-Events don't work, because no event is fired
-Override doesn't work, because of virtual types
-Override of virtual type doesn't work, because it breaks the search result

The only thing that worked so far is hacking the core code. But I really don't want to do that.

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Oct 4, 2017
@leedave
Copy link

leedave commented Oct 4, 2017

@joachimVT

There is a solution, that changes to search operand from OR to AND, you need to create a Module for this

yourVendor/yourmodule/etc/search_request.xml
<?xml version="1.0"?> <requests xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Search/etc/search_request.xsd"> <request query="quick_search_container" index="catalogsearch_fulltext"> <queries> <query xsi:type="boolQuery" name="quick_search_container" boost="1"> <queryReference clause="must" ref="search" /> </query> </queries> </request> </requests>

yourVendor/yourmodule/etc/module.xml
<?xml version="1.0" ?> <config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Module/etc/module.xsd"> <module name="YourVendor_YourModule" setup_version="1.0.0"> <sequence> <module name="Magento_CatalogSearch"/> </sequence> </module> </config>

yourVendor/yourmodule/registration.php
<?php \Magento\Framework\Component\ComponentRegistrar::register( \Magento\Framework\Component\ComponentRegistrar::MODULE, 'YourVendor_YourModule', __DIR__ );

@maghamed
Copy link
Contributor

maghamed commented Oct 4, 2017

Hey, @leedave I would not say that bugs you mentioned are related to Search Customization.
Also, we don't expect that it's a business case to filter by relevance, because relevance is a service value, which we use under the hood, and for different engines, it will have a different value for the same search term given. For example, it could be 0.99 for MySQL and 435 for ElasticSearch. Thus, we don't normalize it and scale. So, applying filter would not give an accurate result. But it makes sense to make Sorting by Relevance, because in the scope of single Search request - all the documents are scaled equally.

Yes, customization of Request.xml is a way to go.

But if you see that Magento has flaws in Core implementation of Fulltext Collection and

The only thing that worked so far is hacking the core code. But I really don't want to do that.

Maybe it makes sense to fix that in a core and make a Code contribution as Pull Request.
Thus, it will help to make Magento 2 better, and not to apply extra-customization.

@korostii
Copy link
Contributor

Hi @maghamed

If you agree that there are flaws to fix in the core, would it not make sense to also reopen this issue and mark it as up for grabs (ro whatever the currebt flow is)?

As @orlangur mentioned elsewhere and I tend to agree in principle, providing pull requests for changes which might get rejected - is potentially quite a waste of time.
And it seems preferrable to have the changes expected being agreed upon publicly, explicitly, and in advance.

Besides, seeing this issue as "Done" in project "branch [develop]" is really confusing.

@amalkarmick
Copy link

I want to add category filter in catalog search. What do I need to do ?
Anyone has any idea ?

@developerlicw
Copy link

Anyone still on this??
I have exactly the same problem.

I am using elasticsearch(ES) with magento2. As you know ES can set its own order, like order by name or id or something else. Now I am working to get my search results in a better order, which is already realized using ES, but magento2 will never give me the correct order, because in this "Magento\CatalogSearch\Model\ResourceModel\Fulltext\Collection", there is a function: "_renderFiltersBefore", and at the last of it, there is:

/*
         * This order is required to force search results be the same
         * for the same requests and products with the same relevance
         * NOTE: this does not replace existing orders but ADDs one more
         */
        $this->setOrder('entity_id');

This caused all my search results disordered. And as I read above posts, I cannot override this "Collection", so does that mean I shouldn't order by search results in ES? That I must use magento to realize any sorting?

Why must magento2 set some order if I have already had it done with ES or other search engines?
Anyone any good idea what should I do?

@rparsi
Copy link

rparsi commented Oct 3, 2018

@igrybkov What should the <query xsi:type="matchQuery" ... look like so that only enabled products are in the quick search result?
I'm using vendor/magento/module-catalog-search/etc/search_request.xml as a reference.
I have checked the Magento Forums and Magento Stack Exchange, nobody knows how to do this.
There's no documentation for this on magento2 dev site either.

I have tried

<query xsi:type="matchQuery" value="$search_term$" name="status">
                <match field="status"/>
                <match field="1" />
            </query>

but I get an Exception:

Query status is not used in request hierarchy in file
vendor/magento/framework/Search/Request/Mapper.php
line 300

@developerlicw
Copy link

@rparsi
Hi, do you mean that you want only products with "status=1" returned? If so, I think your problem can be solved by using "filter" rather than "query".
This page may be a reference: https://www.elastic.co/guide/en/elasticsearch/reference/current/query-filter-context.html
It's document from elasticsearch, but I think you can also find "filter" in your "search_request.xml" like this:
<filter xsi:type="termFilter" name="category_filter" field="category_ids" value="$category_ids$"/> <filter xsi:type="rangeFilter" name="price_filter" field="price" from="$price.from$" to="$price.to$"/> <filter xsi:type="termFilter" name="visibility_filter" field="visibility" value="$visibility$"/>

@rparsi
Copy link

rparsi commented Oct 3, 2018

@developerlicw I want only enabled products in the quick search result. The product boolean attribute status is what indicates if a given product is enabled or not.

I have

<filters>
            <filter xsi:type="termFilter" name="category_filter" field="category_ids" value="$category_ids$"/>
            <filter xsi:type="rangeFilter" name="price_filter" field="price" from="$price.from$" to="$price.to$"/>
            <filter xsi:type="termFilter" name="visibility_filter" field="visibility" value="$visibility$"/>
            <filter xsi:type="termFilter" name="status_filter" field="status" value="1"/>
        </filters>

status_filter is added by me.
However I'm still getting an Exception: Query status is not used in request hierarchy

Am using magento2 2.2.3

@developerlicw
Copy link

@rparsi According to your Exception message, it seems you're still using "status" for query, please check if you have already removed it.

@rparsi
Copy link

rparsi commented Oct 3, 2018

@developerlicw I did remove it. Here is the full content of my etc/search_request.xml file

<?xml version="1.0"?>
<requests xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
          xsi:noNamespaceSchemaLocation="urn:magento:framework:Search/etc/search_request.xsd">
    <request query="quick_search_container" index="catalogsearch_fulltext">
        <dimensions>
            <dimension name="scope" value="default"/>
        </dimensions>
        <queries>
            <query xsi:type="boolQuery" name="quick_search_container" boost="1">
                <queryReference clause="should" ref="search" />
                <queryReference clause="must" ref="category"/>
                <queryReference clause="must" ref="price"/>
                <queryReference clause="must" ref="visibility"/>
            </query>
            <query xsi:type="matchQuery" value="$search_term$" name="search">
                <match field="sku"/>
                <match field="*"/>
            </query>
            <query xsi:type="filteredQuery" name="category">
                <filterReference clause="must" ref="category_filter"/>
            </query>
            <query xsi:type="filteredQuery" name="price">
                <filterReference clause="must" ref="price_filter"/>
            </query>
            <query xsi:type="filteredQuery" name="visibility">
                <filterReference clause="must" ref="visibility_filter"/>
            </query>
            <query xsi:type="filteredQuery" name="status">
                <filterReference clause="must" ref="status_filter"/>
            </query>
        </queries>
        <filters>
            <filter xsi:type="termFilter" name="category_filter" field="category_ids" value="$category_ids$"/>
            <filter xsi:type="rangeFilter" name="price_filter" field="price" from="$price.from$" to="$price.to$"/>
            <filter xsi:type="termFilter" name="visibility_filter" field="visibility" value="$visibility$"/>
            <filter xsi:type="termFilter" name="status_filter" field="status" value="1"/>
        </filters>
        <aggregations>
            <bucket name="price_bucket" field="price" xsi:type="dynamicBucket" method="$price_dynamic_algorithm$">
                <metrics>
                    <metric type="count"/>
                </metrics>
            </bucket>
            <bucket name="category_bucket" field="category_ids" xsi:type="termBucket">
                <metrics>
                    <metric type="count"/>
                </metrics>
            </bucket>
        </aggregations>
        <from>0</from>
        <size>10000</size>
    </request>
</requests>

It's copied from vendor/magento/module-catalog-search/etc/search_request.xml.
Only change is <filter xsi:type="termFilter" name="status_filter" field="status" value="1"/>

@developerlicw
Copy link

developerlicw commented Oct 3, 2018

@rparsi Sorry for the bad answer just now.
After checking the file "vendor/magento/framework/Search/Request/Mapper.php" from which the Exception is thrown, I found the following codes:
$notUsedElements = implode(', ', array_diff($allElements, $mappedElements)); if (!empty($notUsedElements)) { throw new StateException(new Phrase($errorMessage, [$notUsedElements])); }

It seems that exception is caused by unmatched query fields and mapping fields. Maybe you can check whether you've mapped "status" in your mapping.

@rparsi
Copy link

rparsi commented Oct 3, 2018

@developerlicw

Maybe you can check whether you've mapped status in your mapping.

I have no idea how to do that (add a given product field/attribute to the mapping). As I've stated earlier, there's no documentation on this, because magento team thinks developers will magically figure this out without any guidance.

@developerlicw
Copy link

developerlicw commented Oct 3, 2018

@rparsi Agreed with that, magento do provide quite little information about elasticsearch.
As I guess, "status" must be a property of your product, so it's possible to make it indexed in mapping by setting it "filterable"/"filterable in search" in magento admin panel. (Me myself also not aware of the difference, looking forward for more explanation)

If you want to check your mapping, https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-get-mapping.html
The get mapping API elasticsearch provides will be useful.

Besides, it maybe better to access elasticsearch engine and confirm the changes directly, I am using a chrome plugin named "elasticsearch-head" and it's quite useful.

Hope these information helpful to you :)

@rparsi
Copy link

rparsi commented Oct 3, 2018

@developerlicw I'm not using elasticsearch, only MySQL. I don't see anything in admin ui for "filterable in search". The closest is in the product attribute edit form there's an option to enable the status attribute in search results, but the dropdown is disabled (probably because it's a core entity attribute).

This is so frustrating.

@developerlicw
Copy link

developerlicw commented Oct 3, 2018

@rparsi Oh, I've always thought of elasticsearch! My apologies for the misunderstanding, cause I'm using elasticsearch...
Sorry that I've never used MySQL, but I think Magento admin panel may not be such different, the "filterable" setting is under "Store"->"Product", in that page you can find all your products' properties, find "status" and maybe do fixing just for try?

Sorry about being not so helpful, I am also working on magento2, and will inform you if I got some useful clues.

@sai-nekkanti
Copy link

This isn't working in 2.3.3 as well.

@prateeksahus
Copy link

If you are using elasticsearch you have to set the following preference for virtual type in your custom module's di.xml as elasticsearch overrides the virtualtype SearchCollection

<preference for="elasticsearchFulltextSearchCollection" type="Vendor\Module\Model\ResourceModel\Fulltext\Collection" />

next if you face the following error

Fatal error: Uncaught TypeError: Argument 1 passed to Magento\Elasticsearch\Model\Adapter\FieldMapper\Product\AttributeProvider::getByAttributeCode() must be of the type string, null given, called in /var/www/html/magento2/vendor/magento/module-elasticsearch/SearchAdapter/Query/Builder/Sort.php on line 92 and defined in /var/www/html/magento2/vendor/magento/module-elasticsearch/Model/Adapter/FieldMapper/Product/AttributeProvider.php:72
refer to this comment #27112 (comment)

magento-devops-reposync-svc pushed a commit that referenced this issue Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Component: Search Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests