Technical debt : review PrestaShop\PrestaShop\Core\Grid\Search\SearchCriteriaInterface

MeKeyCool commented 2 years ago

Prerequisites

[X] I understand and accept the project's code of conduct
[X] I have already searched in existing features request and found no previous suggestion of this feature.

Hi ! This issue is opened to discuss about PrestaShop\PrestaShop\Core\Grid\Search\SearchCriteriaInterface. I think some refactoring is required but I may need some enlightenments

Is your feature request related to a problem?

Fixing #26218, in https://github.com/PrestaShop/PrestaShop/pull/28782/files#diff-7930ead1cb41932b3be50da6003f2f0159b4ec40ef0fb0fa70f28b4c930db401, I encountered some weird behaviors I had to manage and I think the PrestaShop\PrestaShop\Core\Grid\Search\SearchCriteriaInterface should be refactored.

Describe the solution you'd like

I would like that offset is defined as positive integer with default value at 0 (what would mean "disabling offset" ?) Same for limit : positive value with default at 10.

IMHO, PrestaShop\PrestaShop\Core\Search\Filters::LIST_LIMIT constant should be defined in PrestaShop\PrestaShop\Core\Grid\Search\SearchCriteriaInterface as all default values.

It is just few ideas. Final roadmap should be discussed with @PrestaShop/prestashop-maintainers .

Do you plan to work on this feature?

[X] I'm willing to contribute a formal specification.
[ ] I'm willing to provide any wireframes or design assets required for this feature.
[X] I'm willing to submit a Pull Request that implements this feature.
[X] I'm willing to help verify that the implemented feature works as intended and produces no unintended side effects.

marwachelly commented 2 years ago

Hello @MeKeyCool,

Thank you for your suggestion. The Product Team will take it into consideration for future developments.

Thanks :blush:

MeKeyCool commented 2 years ago

This issue is dedicated to developers.

zuk3975 commented 2 years ago

So I think it depends on one question - if we accept to remove the ability of having unlimited results in grids. Because that is exactly what we would achieve according to your suggestion.

jolelievre commented 2 years ago

I don't think this requires a refacto 🤔 The interface represents the generic contract needed to perform a search on a grid so it has generic methods. Filters class is our main implementation of this interface but it's not the only one, we also have a generic SearchCriteria object which is less advanced than Filters (which are used in automatic build based on parameter resolvers) but is still useful for simple implementations and for modules.

And as @zuk3975 said nothing allows us to state that the limit should always be 10 and the offset 0. For the offset I agree it would make sense, but the default limit depends on use cases and we could have a grid with no limit by default.

Besides, I think you're confusing the default values (which are handled and implemented by the Filters class) from the interface itself which has no notion of default values so far. The interface represents the required parameters at any moment in a grid life, whether it's the initial display that returns the default values or an intermediate state during pagination with specific values it doesn't matter for the interface, it will always need the same methods anyway.

MeKeyCool commented 2 years ago

Sorry if I haven't been clear with my description.

As you said, interface is a contract. In the mentioned fix, I had to consider some "invalid" cases (example : is limit a positive integer ?). I consider that giving a type to an object should ensure the state of the object when I use it (I should not have to test its state when using an instance) so the interface should be more precise about required state.

Further more, defining some specific values for attributes that should be interpreted as a different state make things heavy (cf. https://github.com/PrestaShop/PrestaShop/blob/ad181f7fb89dea67dfebba779a6440854baa6d5b/src/Core/Grid/Search/SearchCriteriaInterface.php#L49-L52 : if it is integer => it's a limit, if it is null => it is not a limit) If the attribute is optional, I recommend to use a dedicated "Option" type as it is done in some other languages

Then allowing an "Optional" value for some attributes induces the question of what should I do if the Option value is None ? Then as it is setup by a hard-coded public const in Filters class, I assumed to keep this strategy with the interface. If you want to allow undefined request attributes, what would mean

undefined OrderBy / OrderWay ? (I don't understand why do we have two functions and no deprecation mention)
undefined Offset ?
undefined Limit ? If we don't give a default value, do we assume that we let this question to DBMS (with all tricky behaviors and debugging situations it would induce) ? IMO it is not a good practice to allow infinite value for limit. If you want to allow overwriting default values, there is lot of solutions (as using functions instead of const).

getFilters(); Isn't clear from the interface. As it is a contract, shouldn't it be typed ? And should we explain use cases in the interface comments ?

PrestaShop / PrestaShop