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

Fixes BaseBuilder getWhere() bug #2232

Merged
merged 4 commits into from
Sep 24, 2019

Conversation

michalsn
Copy link
Member

Description
Fixes #2143 - getWhere() bug with offset.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@jim-parry jim-parry changed the title Fixes #2143 Fixes BaseBuilder getWhere() bug Sep 20, 2019
@jim-parry
Copy link
Contributor

You have added returnSQL and reset parameters to the getWhere() signature.
These don't seem related to the default values bug.
Am I missing something?

@michalsn
Copy link
Member Author

  1. returnSQL - without it this method is not testable.
  2. reset - I feel it should be consistent with the get() method - it just makes more sense to me... so I don't see a reason not to allow reset in this method too.

@jim-parry
Copy link
Contributor

@lonnieezell @MGatner Makes sense. Are you good with this?

@MGatner
Copy link
Member

MGatner commented Sep 24, 2019

It looks good to me, really nice tests. It's unfortunate how all the database & builder methods keep growing in number of parameters but I think @michalsn is right that the current implementation needs those. Maybe down the road some way of globally intercepting DB calls to return SQL could replace some of the individual versions?

@jim-parry
Copy link
Contributor

If we are adding two parameters to a method signature, shouldn't we be updating the user guide too?

@MGatner
Copy link
Member

MGatner commented Sep 24, 2019

Yes, updates to user_guide_src/source/database/query_builder.rst. If @michalsn isn't around I could pass these tonight as a separate PR.

@MGatner
Copy link
Member

MGatner commented Sep 24, 2019

Thanks so much @michalsn, looks great.

@MGatner MGatner merged commit 3a0b8df into codeigniter4:develop Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query Builder getWhere Crash
3 participants