Skip to content

refactor: replace empty() Part 1 #8345

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Dec 28, 2023
Merged

refactor: replace empty() Part 1 #8345

merged 10 commits into from
Dec 28, 2023

Conversation

@kenjis
Copy link
Member

@kenjis kenjis commented Dec 18, 2023 •
edited
Loading

Needs #8344

Description

  • replace empty()

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 kenjis marked this pull request as draft December 18, 2023 07:13
@kenjis kenjis force-pushed the replace-empty branch 4 times, most recently from 102b20c to c666bf7 Compare December 19, 2023 03:04
@github-actions github-actions bot added the stale Pull requests with conflicts label Dec 21, 2023
@github-actions
Copy link

github-actions bot commented Dec 21, 2023

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis added refactor Pull requests that refactor code and removed stale Pull requests with conflicts labels Dec 21, 2023
@kenjis kenjis changed the title refactor: replace empty() refactor: replace empty() Part 1 Dec 22, 2023
@kenjis kenjis marked this pull request as ready for review December 22, 2023 10:58
@github-actions github-actions bot added the stale Pull requests with conflicts label Dec 25, 2023
@github-actions
Copy link

github-actions bot commented Dec 25, 2023

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis removed the stale Pull requests with conflicts label Dec 25, 2023
michalsn
michalsn reviewed Dec 28, 2023
View reviewed changes
system/Model.php
if ($this->tempUseSoftDeletes) {
$builder->where($this->table . '.' . $this->deletedField, null);
} elseif ($this->useSoftDeletes && empty($builder->QBGroupBy) && $this->primaryKey) {
} elseif ($this->useSoftDeletes && ($builder->QBGroupBy === []) && $this->primaryKey) {
Copy link
Member

@michalsn michalsn Dec 28, 2023

Choose a reason for hiding this comment

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

Is () around $builder->QBGroupBy === [] is forced by our tooling? Seems unnecessary.

michalsn
michalsn reviewed Dec 28, 2023
View reviewed changes
system/Model.php
// Some databases, like PostgreSQL, need order
// information to consistently return correct results.
if ($builder->QBGroupBy && empty($builder->QBOrderBy) && $this->primaryKey) {
if ($builder->QBGroupBy && ($builder->QBOrderBy === []) && $this->primaryKey) {
Copy link
Member

@michalsn michalsn Dec 28, 2023

Choose a reason for hiding this comment

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

Same as above.

michalsn
michalsn reviewed Dec 28, 2023
View reviewed changes
system/Test/FeatureTestCase.php

$output = \ob_get_contents();
if (empty($response->getBody()) && ! ($output === '' || $output === false)) {
if (($response->getBody() === null) && ! ($output === '' || $output === false)) {
Copy link
Member

@michalsn michalsn Dec 28, 2023

Choose a reason for hiding this comment

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

Probably unnecessary () around $response->getBody() === null.

michalsn
michalsn reviewed Dec 28, 2023
View reviewed changes
system/Validation/Validation.php
$message = str_replace(
'{param}',
empty($this->rules[$param]['label']) ? $param : lang($this->rules[$param]['label']),
(! isset($this->rules[$param]['label'])) ? $param : lang($this->rules[$param]['label']),
Copy link
Member

@michalsn michalsn Dec 28, 2023

Choose a reason for hiding this comment

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

Do we need () around ! isset($this->rules[$param]['label'])?

michalsn
michalsn reviewed Dec 28, 2023
View reviewed changes
system/View/Parser.php

$fileExt = pathinfo($view, PATHINFO_EXTENSION);
$view = empty($fileExt) ? $view . '.php' : $view; // allow Views as .html, .tpl, etc (from CI3)
$view = ($fileExt === '') ? $view . '.php' : $view; // allow Views as .html, .tpl, etc (from CI3)
Copy link
Member

@michalsn michalsn Dec 28, 2023

Choose a reason for hiding this comment

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

Same here... is it supposed to increase readability? For me, it's the opposite.

Copy link
Member Author

@kenjis kenjis Dec 28, 2023

Choose a reason for hiding this comment

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

In this case, yes.

michalsn
michalsn reviewed Dec 28, 2023
View reviewed changes
system/View/Parser.php

// Remove the () and spaces to we have just the parameter left
$param = ! empty($param) ? trim($param[0], '() ') : null;
$param = ($param !== []) ? trim($param[0], '() ') : null;
Copy link
Member

@michalsn michalsn Dec 28, 2023

Choose a reason for hiding this comment

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

With only one check, I see no added value in using ().

michalsn
michalsn reviewed Dec 28, 2023
View reviewed changes
system/View/View.php
$fileExt = pathinfo($view, PATHINFO_EXTENSION);
// allow Views as .html, .tpl, etc (from CI3)
$this->renderVars['view'] = empty($fileExt) ? $view . '.php' : $view;
$this->renderVars['view'] = ($fileExt === '') ? $view . '.php' : $view;
Copy link
Member

@michalsn michalsn Dec 28, 2023

Choose a reason for hiding this comment

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

Same here

michalsn
michalsn reviewed Dec 28, 2023
View reviewed changes
system/View/View.php
$this->renderVars['view'],
'Views',
empty($fileExt) ? 'php' : $fileExt
($fileExt === '') ? 'php' : $fileExt
Copy link
Member

@michalsn michalsn Dec 28, 2023

Choose a reason for hiding this comment

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

And here

michalsn
michalsn approved these changes Dec 28, 2023
View reviewed changes
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

I left a few comments, but it may be only my preference.

@kenjis
Copy link
Member Author

kenjis commented Dec 28, 2023

There are some unnecessary (), but I merge this for now.

@kenjis kenjis merged commit 838900e into codeigniter4:develop Dec 28, 2023
@kenjis kenjis deleted the replace-empty branch December 28, 2023 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michalsn michalsn michalsn approved these changes

Assignees

No one assigned

Labels

refactor Pull requests that refactor code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kenjis @michalsn