-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Extract strings to constants #7078
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems nice
I think we are using as a key of an array is an implementation detail, what about just removing the prefix? |
3d0d5f9
to
3953849
Compare
There are multiples places where these strings are used, this replaces the scalar strings by constants.
3953849
to
048f2f6
Compare
$sortValues['_sort_order'] = 'ASC'; | ||
$sortValues['_sort_by'] = 'position'; | ||
$sortValues[DatagridInterface::PAGE] = 1; | ||
$sortValues[DatagridInterface::SORT_ORDER] = 'ASC'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use https://github.com/doctrine/collections/blob/1.6.x/lib/Doctrine/Common/Collections/Criteria.php#L18 for ASC
and DESC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this, the phpdoc of the class says:
/**
* Criteria for filtering Selectable collections.
*/
and the usage inside the doctrine
organization: https://github.com/search?q=org%3Adoctrine+Criteria%3A%3ADESC&type=Code looks like it's only around the Criteria
class.
Thnk you @franmomu |
Subject
Some polishing, there are multiples places where these strings are used, this replaces the scalar strings by constants.
Since they seem related to
Datagrid
, I've created them inDatagridInterface
.I've chosenKEY_
prefix, but any other name is welcome.I am targeting this branch, because these changes are BC.
Changelog