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

Adds a when() method to the query builder. #6329

Closed
wants to merge 278 commits into from
Closed

Adds a when() method to the query builder. #6329

wants to merge 278 commits into from

Conversation

lonnieezell
Copy link
Member

@lonnieezell lonnieezell commented Aug 2, 2022

Description

This adds a method, when() to the Query Builder that can conditionally modify queries based on the truthiness of a given condition.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis
Copy link
Member

kenjis commented Aug 2, 2022

This is an enhancement, so it should go to 4.3 branch, not develop.

@kenjis kenjis added enhancement PRs that improve existing functionalities database Issues or pull requests that affect the database layer labels Aug 2, 2022
@lonnieezell lonnieezell changed the base branch from develop to 4.3 August 2, 2022 21:52
@lonnieezell lonnieezell changed the base branch from 4.3 to develop August 2, 2022 21:53
@lonnieezell lonnieezell changed the base branch from develop to 4.3 August 3, 2022 03:59
@kenjis
Copy link
Member

kenjis commented Aug 3, 2022

@lonnieezell Only changing the base branch on GitHub will not work.
Because you created the PR branch based on develop.

Even if you rebase, if there are commits that have not been merged into 4.3, the PR branch will contain them.

The easy way is to create a new branch from 4.3 and cherry-pick commits that are needed.

@datamweb
Copy link
Contributor

datamweb commented Aug 3, 2022

@kenjis , this is a question for me too, if the user has chosen the wrong branch and opens PR. How can he correct it? Please add the necessary explanations to file workflow if possible.

@kenjis
Copy link
Member

kenjis commented Aug 3, 2022

@datamweb
If you have the PR branch feat-abc:

  1. Create a new branch feat-abc-new from the correct branch, and cherry-pick the commits you did.
  2. Delete the PR branch feat-abc, and rename the new branch feat-abc-new to feat-abc.
  3. Force push.
  4. On GitHub PR page, change the base branch to the correct branch.

@datamweb
Copy link
Contributor

datamweb commented Aug 3, 2022

Great, got it.
Thanks.

@kenjis
Copy link
Member

kenjis commented Aug 12, 2022

@datamweb I sent a PR #6372

kenjis and others added 3 commits August 21, 2022 11:23
Co-authored-by: MGatner <[email protected]>
GET/POST data and the Validation Errors are used together when a validation error occurs.
Therefore, it is not bad that withInput() also saves the validation errors.
@lonnieezell
Copy link
Member Author

@kenjis fixed to point toward 4.3 now

@kenjis kenjis added the 4.3 label Aug 21, 2022
Comment on lines 19 to 24
- Due to a bug fix, now :php:func:`random_string()` with the first parameter ``'crypto'`` throws ``InvalidArgumentException`` if the second parameter ``$len`` is an odd number.
- Due to a bug fix, now :php:func:`random_string()` with the first parameter 'crypto' throws InvalidArgumentException if the second parameter $len is an odd number.

Enhancements
************

none.
Copy link
Member

Choose a reason for hiding this comment

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

Remove this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

/**
* Only runs the query when $condition evaluates to true
*
* @param mixed $condition
Copy link
Member

Choose a reason for hiding this comment

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

Don't use mixed. See #6310

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

/**
* @var MockConnection
*/
protected $db;
Copy link
Member

Choose a reason for hiding this comment

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

Use property type, instead of @var.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kenjis I'm not sure exactly what you're looking for here....

@kenjis
Copy link
Member

kenjis commented Aug 21, 2022

@lonnieezell
Copy link
Member Author

Well, looks like pulling in the changes from remote screwed me over here. Will fix that tomorrow.

@kenjis kenjis added the stale Pull requests with conflicts label Sep 10, 2022
@iRedds
Copy link
Collaborator

iRedds commented Sep 15, 2022

I was sure I left a comment on this PR, but for some reason I didn't find it. I must have gotten too old. =)

I would like to suggest reconsidering the idea of this feature.

The when method is not logically related to databases and can be applied to the framework API.
It seems to me that it would be more convenient to put this method into a separate trait, which can be added to such classes as Request, Response, Model, and other classes.

In addition to the when method, you can also add the whenNot method.

@lonnieezell
Copy link
Member Author

lonnieezell commented Sep 23, 2022

The when method is not logically related to databases and can be applied to the framework API.
It seems to me that it would be more convenient to put this method into a separate trait, which can be added to such classes as Request, Response, Model, and other classes.

In addition to the when method, you can also add the whenNot method.

@iRedds I think that's a great idea. Will do.

@lonnieezell
Copy link
Member Author

Closing this as it's been replaced by #6574

@paulbalandan paulbalandan deleted the db-when branch October 30, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 database Issues or pull requests that affect the database layer enhancement PRs that improve existing functionalities stale Pull requests with conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.