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

When/WhenNot methods for db in a trait #6574

Merged
merged 7 commits into from
Sep 30, 2022
Merged

When/WhenNot methods for db in a trait #6574

merged 7 commits into from
Sep 30, 2022

Conversation

lonnieezell
Copy link
Member

@lonnieezell lonnieezell commented Sep 23, 2022

This replaces #6329. It adds a new ConditionalTrait to CodeIgniter that can be used to add when() and whenNot() methods in various places in the framework. This PR adds it to the database layer for inline conditions to queries.

Instead of:

$query = $this->where('foo', $foo);

if ($bar) {
    $query = $query->where('bar', $bar);
}

return $query->findAll();

you can now do:

return $query->where('foo', $foo)
    ->when($bar, static function($query) {
        return $query->where('bar', $bar);
    })
    ->findAll();

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

@paulbalandan paulbalandan changed the base branch from develop to 4.3 September 23, 2022 05:18
@kenjis kenjis added enhancement PRs that improve existing functionalities wrong branch PRs sent to wrong branch and removed wrong branch PRs sent to wrong branch labels Sep 23, 2022
@kenjis kenjis added the database Issues or pull requests that affect the database layer label Sep 23, 2022
@iRedds
Copy link
Collaborator

iRedds commented Sep 23, 2022

What do you think of this kind of code?

function when ($when, callable $true, ?callable $false = null)
{
	($when ? $true : $false ?? function(){})($this, $when);
}

function whenNot ($when, callable $true, ?callable $false = null)
{
	when($when, $false ?? function(){}, $true);
}

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I like this change! Good suggestion from @iRedds. A agree with @kenjis' tweaks but otherwise approve.

system/Traits/ConditionalTrait.php Outdated Show resolved Hide resolved
system/Traits/ConditionalTrait.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Member

MGatner commented Sep 23, 2022

What do you think of this kind of code?

@iRedds I like the efficiency but I had to read it like five times to figure it out; I think @lonnieezell's version is much easier to understand without galaxy brain 😉

I do like whenNot() simply being an inversion of when() instead of a repeat of the code - less to break in future changes. That would be a good change to apply here.

@kenjis
Copy link
Member

kenjis commented Sep 23, 2022

@iRedds To be honest, the code is difficult to understand.

Code should be written to minimize the time it would take for someone else to understand it.

@kenjis kenjis added the stale Pull requests with conflicts label Sep 25, 2022
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I follow @iRedds suggestion to make this one method with its inversion.

system/Traits/ConditionalTrait.php Show resolved Hide resolved
@kenjis kenjis added the 4.3 label Sep 25, 2022
system/Traits/ConditionalTrait.php Show resolved Hide resolved
*
* @return $this
*/
public function whenNot($condition, callable $callback, ?callable $defaultCallback = null): self
Copy link
Member

Choose a reason for hiding this comment

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

whenNot as the inverse of when seems quite not natural for me. It's a bit off. How about other antonyms of when:

  • except
  • unless
  • if not
  • lest

Copy link
Member Author

Choose a reason for hiding this comment

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

The method expects the condition to evaluate to false, which makes except and unless not make semantic sense. ifNot as an opposite of when feels wrong also. lest feels like the condition should evaluate to true to me, also.

whenNot does at least match the pattern of other methods like where/whereNot. I'm open to suggestions but I'm not sure any of these fit, unless we change the whole method to suit the name.

Copy link
Member

Choose a reason for hiding this comment

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

except/unless makes sense to me if the statement is read in a specific way. In the context of the DB builder, since I know the usage of when then when I read its inverse (except/unless):

$builder = $this->db->table('users');
$user = model('Users')->find(1);

return $builder->except($user, fn ($query) { /* some code */ })->get();

In this example, I would read the code as "Except when $user is truthy (or a found user record), let the $builder execute the callable then get the DB result."

Copy link
Member

Choose a reason for hiding this comment

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

whenNot is easy to understand for non native English speakers.
It is logical. The when whenNot pair is easy to remember, because we already have where/whereNot.

except is difficult to imagine as the opposite of when.

kenjis
kenjis previously approved these changes Sep 27, 2022
@kenjis kenjis dismissed their stale review September 27, 2022 02:42

There is a conflict.

@lonnieezell
Copy link
Member Author

@MGatner are we ok to merge this one?

@MGatner
Copy link
Member

MGatner commented Sep 28, 2022

@lonnieezell It needs to be switched to 4.3. You can try this:

git rebase --onto develop 4.3 when-trait

Just be ready to git rebase --abort if things go poorly 😅 and cherry-pick into a new branch.

I think the content is all good, though I do think @paulbalandan 's suggestion is good. Maybe he would add it as an actual GitHub suggested change?

@lonnieezell
Copy link
Member Author

@MGatner It's already on 4.3 so we're good there. Just committed @paulbalandan suggestion.

Copy link
Member

@MGatner MGatner 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! Last comment is the negative name; I don't have a strong feeling either way.

@kenjis kenjis removed the stale Pull requests with conflicts label Sep 29, 2022
@kenjis
Copy link
Member

kenjis commented Sep 29, 2022

Can you remove the merge commit 49f1e70 ?
We don't need merge commits in PR branches.

@kenjis
Copy link
Member

kenjis commented Sep 29, 2022

@lonnieezell
Copy link
Member Author

Can you remove the merge commit 49f1e70 ?

I think the likelihood of me messing something up trying to do that is pretty high, honestly. Seems we just squash and merge and the history is nice and clean that way, won't it be?

@kenjis
Copy link
Member

kenjis commented Sep 29, 2022

Okay, I rebased and force-pushed.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Hurrah!

Copy link
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

Finally, good result, thank you all.😍

@lonnieezell lonnieezell merged commit eb21383 into 4.3 Sep 30, 2022
@lonnieezell lonnieezell deleted the when-trait branch September 30, 2022 04:18
@MGatner
Copy link
Member

MGatner commented Sep 30, 2022

🥳

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants