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

Refactor: Apply PHPStan rule "Short ternary operator is not allowed" to RouteCollection #7947

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

pjsde
Copy link
Contributor

@pjsde pjsde commented Sep 16, 2023

Apply PHPStan rule "Short ternary operator is not allowed" to RouteCollection

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 added the refactor Pull requests that refactor code label Sep 16, 2023
@@ -1699,7 +1700,9 @@ public function resetRoutes()
*/
protected function loadRoutesOptions(?string $verb = null): array
{
$verb = $verb ?: $this->getHTTPVerb();
if (null === $verb || $verb === '') {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need $verb === '' check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because $verb is a string we also need to validade if its empty string to replace the validation $verb = $verb ?: $this->getHTTPVerb(); unless we check like this:

if (! $verb) {
    $verb = $this->getHTTPVerb();
}

what do you sugest?

Copy link
Member

Choose a reason for hiding this comment

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

Then, what if '0'?
Probably we don't expect '0' for $verb.

I'm not sure what is correct. The question is just a question.
So if you believe you should check '', it is fine.

By the way, we do not use Yoda conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After another review of all uses of this method I think its reasonable to only validate the nullvalue.

I will fix the commit.

@pjsde pjsde force-pushed the refactor-routecollection-phpstan branch from f6899f9 to d736bcf Compare September 18, 2023 09:40
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions github-actions bot added the stale Pull requests with conflicts label Sep 20, 2023
@kenjis kenjis removed the stale Pull requests with conflicts label Sep 20, 2023
@codeigniter4 codeigniter4 deleted a comment from github-actions bot Sep 20, 2023
@kenjis kenjis merged commit 2ad658d into codeigniter4:develop Sep 20, 2023
59 of 60 checks passed
@pjsde pjsde deleted the refactor-routecollection-phpstan branch September 20, 2023 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants