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

Fix support for BETWEEN predicate in database class #6292

Merged

Conversation

SeriousKen
Copy link
Contributor

@SeriousKen SeriousKen commented Feb 24, 2024

This PR …

Fixes

Using BETWEEN in a where was not supported in the 3 argument form (eg. Db::table('products')->where('quantity', 'BETWEEN', [10, 20])) this PR fixes this.

Breaking changes

None

Docs

You can now use Db::table('products')->where('quantity', 'BETWEEN', [$min, $max]) to add a BETWEEN clause to a query.

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

Fixes BETWEEN predicate for SQL queries.
Fix to support BETWEEN predicate in database query abstraction.
@distantnative
Copy link
Member

@SeriousKen don't worry about the failing code styling test. We changed something on our CI yesterday and the reviewdog action seems to be broken right now.

@distantnative distantnative added the type: bug 🐛 Is a bug; fixes a bug label Feb 24, 2024
@distantnative distantnative added this to the 4.2.0 milestone Feb 24, 2024
Copy link
Member

@distantnative distantnative left a comment

Choose a reason for hiding this comment

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

Looks good to me (thanks!). Leaving the final review to @lukasbestle as he knows those SQL classes much better.

@afbora
Copy link
Member

afbora commented Feb 24, 2024

BETWEEN operator also supports text values like BETWEEN 'Foo' AND 'Bar'. I just wonder that is this PR also supports strings?

@SeriousKen
Copy link
Contributor Author

BETWEEN operator also supports text values like BETWEEN 'Foo' AND 'Bar'. I just wonder that is this PR also supports strings?

I can't see any reason why it would not support strings as it ultimately gets translated into to a prepared statement with parameters.

@afbora
Copy link
Member

afbora commented Feb 24, 2024

Great! So could you add unit test for strings please?

@SeriousKen
Copy link
Contributor Author

Great! So could you add unit test for strings please?

No problem, test added

Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@distantnative distantnative merged commit 5c3e69e into getkirby:develop-minor Feb 24, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants