Improve \Magento\Framework\Api\SortOrder

[Vinai]

Purpose

Make the specification of sort directions on sort criteria easier for developers.

Background

This PR addresses several issues:

PHPDoc type hint mismatch
All parameter type hints expecting a sort direction specify @param string $direction, but the constants SearchCriteriaInterface::SORT_ASC and SearchCriteriaInterface::SORT_DESC are integers.
As a developer, when I see a method setDirection() that takes a string, I expect the argument to be either ASC or DESC. Neither work, but there is no error helpful error message.

Unintuitive placement of constants
Also the location of the constants is not intuitive. When working with the code in question, a developer is focused on the classes SortOrder and SortOrderBuilder. So the expectation would be to find the class constants on either of those classes.
The addition of the methods setAscendingDirection() and setDescendingDirection() to the SortOrderBuilder removes the need for developers to know the location of constants at all.

Missing validation
The sort direction can be specified as a scalar argument, but there is no validation.
This PR adds validation and throws an explanatory exception on validation failure.

Reasons for this PR

I simply hope to make it easier and more intuitive for developers to specify a sort order.

[kokoc]

@Vinai thank you for the contribution! Internal reference: MAGETWO-39814

[kokoc]

@Vinai we have completed the code analysis. Here are outcomes:

Classes from API directories are used during wsdl creation process and REST documentation creation process. Particularly, all getters(methods with "get", "is" or "has" prefixes) from such classes are used to build WebAPI representation for appropriate entities. So, this pull request introduces two additional fields("ascending" and "descending") to API and documentation.

Also your pull request shown us that in some cases internal PHP api could be richer than Web API. Currently, framework doesn't support such feature, so we forced to reject this contribution.

[Vinai]

Thank you for your feedback!
Is it possible to split the PR into several smaller ones that might work?
For example, fixing the PHPDoc type hint mismatch and changing the placement of the constants and the constant values would not cause the PHP API to be richer then the Web API.
The more expressive methods are only one part of this PR.
If you agree, I'll be happy to remove those conflicting parts of the PR.

[Vinai]

I've updated the PR so it no longer includes the new isAscending() and isDescending() methods. Now no new methods are added. The remaining features of the PR are

• Use ASC and DESC as the sort direction constant values instead of -1 and 1 to match the phpdoc type hint and also make it more intuitive to use.
• Move sort direction constants into SortOrder class to make them more easy to find.
• Add validation of sort direction input type and value to improve developer experience.
• Make input strings case insensitive, so asc and desc work as well, for ease of use.
• Add unit test coverage to the SortOrder class.

Please reevaluate the PR.

[Vinai]

The failing integration test \Magento\Checkout\Controller\OnepageTest::testSaveOrderActionWithoutFormKey() is green locally with PHP 5.5 and PHP 5.6.
I can't debug why it is failing without being able to reproduce the failing test. I think it probably is not related to the PR because otherwise it would fail locally, too.

[kokoc]

@Vinai Thank you for fixing the PR. I will get back to you once we complete the analysis.

[kokoc]

@Vinai the code looks good. Could you please update pull request with latest changes from mainline? Conflicts block travis from fetching new commits, so build fails.

[Vinai]

Thanks, will do

[Vinai]

Rebased onto current develop, all unit tests are green. However since the last pull I have hundreds of failing integration tests locally (in the develop branch, so independent of this PR). Will need some time to debug and try to get them running again. I'll push the PR branch never the less, because I'm curious what travis says.

[Vinai]

All green - well, I guess that's good for the PR then. I'll just have to debug what is not working locally again.