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

Improve how ordering param is provided in short URL list endpoint #836

Closed
acelaya opened this issue Sep 13, 2020 · 0 comments · Fixed by #840
Closed

Improve how ordering param is provided in short URL list endpoint #836

acelaya opened this issue Sep 13, 2020 · 0 comments · Fixed by #840
Milestone

Comments

@acelaya
Copy link
Member

acelaya commented Sep 13, 2020

Currently, the ordering can be provided in two ways:

  • ?orderBy=shortCode: This allows defining the filed to sort by. The direction is assumed to be ASC
  • ?orderBy[shortCode]=DESC: This allows defining both the field and the direction, being the field a key and the direction a value, once PHP parses the query params.

This approach has one benefit. PHP parses both of them out of the box, but it has two problems:

  • The value has to be treated as string|array, forcing checks in order to know how the value was provided.
  • The second approach is not 100% standard http, which makes you have to URL-encode the values, like this ?orderBy%5BshortCode%5D=DESC.

In order to simplify that, the two supported values should be these:

  • ?orderBy=shortCode
  • ?orderBy=shortCode-DESC (using a hyphen or some other character to separate the field name from the direction).

This would be standard HTTP, and would also be simpler to parse, as we can always consider it a string which needs to be exploded, being the second param optional and falling back to ASC.

$parts = explode('-', $orderBy);

$orderField = $parts[0];
$orderDir = $parts[1] ?? 'ASC';

The orderBy[shortCode]=DESC approach will be considered deprecated, removed from docs, and removed from Shlink by v3.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant