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

Default ORDER BY for all entities and their relationships #14685

Merged
merged 5 commits into from
Jan 12, 2023

Conversation

coldic3
Copy link
Contributor

@coldic3 coldic3 commented Jan 4, 2023

Q A
Branch? 1.11
Bug fix? yes (fixes sorting rows for postgres)
New feature? yes (adds default order by)
BC breaks? no
Deprecations? no
Related tickets
License MIT

This PR introduces OrderByIdentifierSqlWalker and adds the default order by to the ORM configurations for all relationships.

@coldic3 coldic3 requested a review from a team as a code owner January 4, 2023 15:27
@coldic3 coldic3 force-pushed the feature/order-by-id-by-default branch 3 times, most recently from daf1db4 to 25b5c3c Compare January 5, 2023 10:26
@coldic3 coldic3 added the Feature New feature proposals. label Jan 5, 2023
Copy link
Member

@GSadee GSadee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether this should be regarded as a bug fix or a new feature. In theory, this PR adds new functionality but also fixes an inconsistency in how data is loaded between different database engines, which fails some builds. I would probably be in favour of opening this to 1.11, but I'm curious to hear other people's opinions 🤔

@GSadee
Copy link
Member

GSadee commented Jan 9, 2023

What is more, if the base branch were to be changed (or not only then), it would be nice to note this change in the proper UPGRADE files.

@coldic3 coldic3 changed the title Default ORDER BY for all entities and their relationships [WIP] Default ORDER BY for all entities and their relationships Jan 10, 2023
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Jan 10, 2023
@coldic3 coldic3 changed the title [WIP] Default ORDER BY for all entities and their relationships Default ORDER BY for all entities and their relationships Jan 10, 2023
@coldic3 coldic3 force-pushed the feature/order-by-id-by-default branch from 283c2f9 to 5ee52d0 Compare January 10, 2023 10:36
@coldic3 coldic3 changed the base branch from 1.13 to 1.11 January 10, 2023 10:36
@coldic3 coldic3 removed the Feature New feature proposals. label Jan 10, 2023
@coldic3 coldic3 force-pushed the feature/order-by-id-by-default branch from 5ee52d0 to 2cffba8 Compare January 11, 2023 07:52
@coldic3 coldic3 changed the title Default ORDER BY for all entities and their relationships [WIP] Default ORDER BY for all entities and their relationships Jan 11, 2023
@coldic3 coldic3 force-pushed the feature/order-by-id-by-default branch from 62e2c1e to 53caa2f Compare January 11, 2023 09:16
@coldic3 coldic3 changed the title [WIP] Default ORDER BY for all entities and their relationships Default ORDER BY for all entities and their relationships Jan 11, 2023
@GSadee GSadee added the Bug Confirmed bugs or bugfixes. label Jan 12, 2023
@GSadee GSadee merged commit 7d8a347 into Sylius:1.11 Jan 12, 2023
@GSadee
Copy link
Member

GSadee commented Jan 12, 2023

Thanks, Kevin! 🥇

coldic3 added a commit that referenced this pull request Jan 12, 2023
… all entities (GSadee)

This PR was merged into the 1.12 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12|
| Bug fix?        | no                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no|
| Related tickets | after #14685 |
| License         | MIT                                                          |

<!--
 - Bug fixes must be submitted against the 1.11 or 1.12 branch
 - Features and deprecations must be submitted against the 1.13 branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

3dbecb7 Add note to UPGRADE-1.12 file about default order by for all entities
@coldic3 coldic3 deleted the feature/order-by-id-by-default branch January 12, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants