magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.39k stars 9.29k forks source link

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

Closed joachimVT closed 7 years ago

joachimVT commented 7 years ago

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>
3. Add new Collection.php file and override _renderFiltersBefore() method

<?php

namespace Sanimarkt\App\Model\ResourceModel\Fulltext;

/**

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 commented 7 years ago

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" />
maghamed commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@igrybkov Any comment on this ?

leedave commented 6 years ago

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.

leedave commented 6 years ago

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

`

yourVendor/yourmodule/etc/module.xml `<?xml version="1.0" ?>

` yourVendor/yourmodule/registration.php `
maghamed commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

developerlicw commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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="visibility_filter" field="visibility" value="$visibility$"/>`
rparsi commented 5 years ago

@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 commented 5 years ago

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

rparsi commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 4 years ago

This isn't working in 2.3.3 as well.

prateeksahus commented 1 year ago

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 https://github.com/magento/magento2/issues/27112#issuecomment-748916677