Fix LIMIT_UNLIMITED type issue

[bckpff]

This fixes a type issue with the \Pimcore\Bundle\EcommerceFrameworkBundle\IndexService\ProductList\ElasticSearch\AbstractElasticSearch::LIMIT_UNLIMITED constant.

The external setter for the limit only accepts integer values thus preventing the use of the elastic scroll api and limiting result sets to 10.000 (default configuration) or the value max_result_window.

I have read the CLA Document and I hereby sign the CLA

[kingjia90]

Could you please rebase the branch to 1.0 as this should go into a bug fix release? TIA

[kingjia90]

Having some doubts on a bigger picture:

• should be it accepting string|int $limit instead https://github.com/pimcore/ecommerce-framework-bundle/blob/7ca7952c068c83be62ed42457d561c3cdb112745/src/IndexService/ProductList/ElasticSearch/AbstractElasticSearch.php#L330 and consider it as a regression from https://github.com/pimcore/pimcore/pull/13474?
• was unlimited intentionally introduced to have a different behavior from -1 WITHOUT this scroll request?

[bckpff]

was unlimited intentionally introduced to have a different behavior from -1 WITHOUT this scroll request?

I don't see the use case for that. Elasticsearch discourages fetching result sets with size (+ offset) > max_result_window and explicitly states to use the scroll API (or better the search_after method: https://www.elastic.co/guide/en/elasticsearch/reference/current/paginate-search-results.html#search-after)

Maybe add a setter and getter for the doScrollRequest Property?

[github-actions[bot]]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[fashxp]

@kingjia90 I think proposed solution is fine and should not do any harm.

@bckpff could you please once more add a comment to sign the CLA ... not sure if it works already in PR description. thank you very much.

[bckpff]

I have read the CLA Document and I hereby sign the CLA

[kingjia90]

@kingjia90 I think proposed solution is fine and should not do any harm.

There's a potential bc-break in the native parameter type, if they passed unlimited as string is going to crash, we never deprecated it nor mentioned in the upgrade notes, it's more to consider that the PHPDoc (only int) was wrong.

Probably could refactor by removing the constant, and just stick with if ($limit > 0) or so to keep it simpler.

[fashxp]

@kingjia90 I think proposed solution is fine and should not do any harm.

There's a potential bc-break in the native parameter type, if they passed unlimited as string is going to crash, we never deprecated it nor mentioned in the upgrade notes, it's more to consider that the PHPDoc (only int) was wrong.

Probably could refactor by removing the constant, and just stick with if ($limit > 0) or so to keep it simpler.

But that bc break already happened with Pimcore 11.0 right? So in Pimcore 11 it never worked so far (neither with the constant nor with a custom string). And we're fixing it now at least for the case when you used the constant.

[kingjia90]

ah ok, got it, then merging soon