feat: add IncomingRequest::is() method #6995

kenjis · 2022-12-20T07:09:25Z

Description
Writing code like strtolower($this->request->getMethod()) !== 'post' is a pain.

add IncomingRequest::is()

$request->is('get')
$request->is('post')
$request->is('put')
$request->is('delete')
$request->is('head')
$request->is('patch')
$request->is('options')
$request->is('json')
$request->is('ajax')

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

datamweb

To be beautiful code.😄

michalsn · 2022-12-20T07:41:03Z

system/HTTP/IncomingRequest.php

+            return strtolower($this->getMethod()) === $value;
+        }
+
+        if ($value === 'json') {


Not sure if json belong here. But if we will have it, why not also have ajax? Basically the same story.

I'm fine to add ajax.

We already have the isAJAX() method.

Yes, we have, and as you may notice, we use it. IMO proposed is() method would serve as a convenient shortcut, so I don't see a problem with another way of accessing the same method.

iRedds · 2022-12-20T09:06:10Z

public function isMethod(string $method) : bool {}
public function isGet() : bool {}
public function isPost() : bool {}
// e.t.c.

the default method name is in uppercase.
The fact that in the framework the method name is in lowercase is the wrong way.

kenjis · 2022-12-20T09:41:53Z

The fact that in the framework the method name is in lowercase is the wrong way.

What do you mean? Why is lowercase wrong way?

iRedds · 2022-12-20T12:10:40Z

The fact that in the framework the method name is in lowercase is the wrong way.

What do you mean? Why is lowercase wrong way?

Because "the default method name is in uppercase."
Converting to lowercase is an overhead.

MGatner · 2022-12-20T12:18:56Z

I think @iRedds was pointing out that we only need this method because we are inconsistent with our case in the getMethod() return. The $upper parameter is already deprecated which means we are currently forcing lowercase method names. However if we want to go PSR-7 we cannot modify the case:

     * While HTTP method names are typically all uppercase characters, HTTP
     * method names are case-sensitive and thus implementations SHOULD NOT
     * modify the given string.

I'm not sure the best approach here. I am not opposed to this helper method, but I also believe it should be unnecessary.

michalsn · 2022-12-20T12:38:00Z

The lowercase/uppercase "problem" was indeed created by PSR-7 compatibility. I don't see a way to resolve it without introducing quite a big BC break because, by default, everything is in lowercase now (and was the same way in CI3).

kenjis · 2022-12-20T22:14:45Z

Because "the default method name is in uppercase."

The method token is case-sensitive because it might be used as a gateway to object-based systems with case-sensitive method names. By convention, standardized methods are defined in all-uppercase US-ASCII letters.
https://www.rfc-editor.org/rfc/rfc9110#name-overview

It seems method names should be case-sensitive.

The getMethod() returns lowercase now.
So strtolower($this->getMethod()) === $value; is redundant now.
But the default $upper = false in getMethod() will be removed,
so we should not depend on it.

kenjis · 2022-12-21T08:43:31Z

@iRedds Do you say that the standard HTTP method names are in uppercase?
I changed the implementation a bit.

$ curl -X get https://www.w3.org/ -i
HTTP/2 400 
server: cloudflare
date: Wed, 21 Dec 2022 08:40:47 GMT
content-type: text/html
content-length: 155
cf-ray: -

<html>
<head><title>400 Bad Request</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<hr><center>cloudflare</center>
</body>
</html>

MGatner · 2022-12-21T12:17:13Z

I think case-sensitivity is in our favor. We don't need to adjust the case on the method return because Get is not GET. Forcing these to lower does appear to be a violation of the spec; maybe we add a new method for now?

kenjis · 2022-12-22T00:20:55Z

The lowercase/uppercase "problem" was indeed created by PSR-7 compatibility. I don't see a way to resolve it without introducing quite a big BC break because, by default, everything is in lowercase now (and was the same way in CI3).

Yes, the current getMethod() returns values that are converted to lowercase, and it seems a violation of the HTTP specification.

The behavior should be fixed but it is a big BC break. So it is better that we wait for v5.0.

@MGatner This is() method is the new method to check HTTP methods in case insensitive manner.

michalsn · 2022-12-22T06:48:04Z

The behavior should be fixed but it is a big BC break. So it is better that we wait for v5.0.

I agree with this.

is() may serve as a handful helper method, and since it handles json and ajax it's not 100% redundant with getMethod().

kenjis · 2022-12-22T09:10:22Z

Rebased and resolved, and all green!

kenjis · 2022-12-22T10:13:30Z

Added docs.

user_guide_src/source/changelogs/v4.3.0.rst

MGatner

I do t feel great about it but I agree with the logic that landed here. Definitely we should fix all this in v5.

Also, we need to move away from our AJAX methods as they are less and less reliable; maybe in the future we can provide some different terms for the same concept.

michalsn · 2022-12-22T14:21:38Z

next major version?

@MGatner I'm not sure if this label was meant to be added to this PR, but if so, I would have second thoughts about this PR. v5 means that we already have the getMethod() fixed, so is() method wouldn't make as much sense to me.

Then, I would rather add the isJSON() method.

MGatner · 2022-12-22T14:23:39Z

@michalsn I've been using that label for PRs or threads I want to come back and review for v5 content. It doesn't necessarily mean "this PR belongs in v5" (though sometimes) but more like "read this thread for a relevant v5 idea".

Rector not to delete the code.

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

kenjis · 2022-12-23T04:36:11Z

Rebased and pushed two commits.

We need the same method in CLIRequest because Controller can be run via CLI.

kenjis · 2022-12-25T00:50:07Z

Then, I would rather add the isJSON() method.

I found isJSON() method. It was deprecated in 4.0.5.
See https://github.com/codeigniter4/CodeIgniter4/pull/7019/files#diff-2cab244a084ce08ac666dff9a88aee13003b4170f6c02a690263e3260dac26d8R35

And it was removed in 4.1.2.
See 55b43083

Ah, it was added in 4.0.5.
https://github.com/codeigniter4/framework/blob/v4.0.5/system/HTTP/Message.php#L143

michalsn · 2022-12-25T08:19:00Z

Well... it's strange to me why we deprecated isJSON()... and now we're, in fact, introducing it again. I tried to follow the related discussions, but I still have no point. Anyway, I don't mind it at all. This is a common functionality that will make the life of developers easier. At least, in my opinion.

MGatner · 2022-12-26T12:35:22Z

I think isJSON() was removed when we were trying to establish PSR-7 compatibility, but before we split framework-specific methods out into traits. At least that's my best recollection. It's convenient, I have no problem with it, but it is one more framework-coupling method that doesn't save on lines of code or much readability.

kenjis added enhancement PRs that improve existing functionalities 4.3 labels Dec 20, 2022

datamweb reviewed Dec 20, 2022

View reviewed changes

michalsn reviewed Dec 20, 2022

View reviewed changes

kenjis force-pushed the feat-request-is branch 2 times, most recently from 4f364b4 to 5848832 Compare December 21, 2022 08:22

michalsn approved these changes Dec 22, 2022

View reviewed changes

kenjis force-pushed the feat-request-is branch from 5848832 to ce5e3ff Compare December 22, 2022 06:55

kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Dec 22, 2022

kenjis force-pushed the feat-request-is branch from ce5e3ff to 5963b26 Compare December 22, 2022 08:26

datamweb reviewed Dec 22, 2022

View reviewed changes

user_guide_src/source/changelogs/v4.3.0.rst Outdated Show resolved Hide resolved

MGatner approved these changes Dec 22, 2022

View reviewed changes

MGatner added the next major version? Read this for a relevant v5 idea label Dec 22, 2022

kenjis added 3 commits December 23, 2022 13:27

feat: add IncomingRequest::is() method

d8f51d8

docs: change @phpstan-param

def2306

Rector not to delete the code.

test: add test for invalid value

97c539c

kenjis and others added 6 commits December 23, 2022 13:27

feat: support is('ajax')

2ca4ece

fix: HTTP method names should be uppercase

624e7b3

docs: add docs

e30e981

docs: fix typo

0007146

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

refactor: rename parameter name

2d97803

docs: add explanation

08e9377

kenjis force-pushed the feat-request-is branch from 5111dce to 08e9377 Compare December 23, 2022 04:29

kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Dec 23, 2022

feat: add CLIRequest::is()

a338610

We need the same method in CLIRequest because Controller can be run via CLI.

MGatner approved these changes Dec 24, 2022

View reviewed changes

kenjis merged commit 8f3cf8b into codeigniter4:4.3 Dec 24, 2022

kenjis deleted the feat-request-is branch December 24, 2022 23:15

kenjis mentioned this pull request Jan 17, 2023

docs: use Request::is() instead of getMethod() #7137

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add IncomingRequest::is() method #6995

feat: add IncomingRequest::is() method #6995

kenjis commented Dec 20, 2022 •

edited

Loading

datamweb left a comment

michalsn Dec 20, 2022

kenjis Dec 20, 2022

kenjis Dec 20, 2022

iRedds Dec 20, 2022

michalsn Dec 20, 2022

iRedds commented Dec 20, 2022

kenjis commented Dec 20, 2022

iRedds commented Dec 20, 2022

MGatner commented Dec 20, 2022

michalsn commented Dec 20, 2022

kenjis commented Dec 20, 2022 •

edited

Loading

kenjis commented Dec 21, 2022

MGatner commented Dec 21, 2022

kenjis commented Dec 22, 2022

michalsn commented Dec 22, 2022

kenjis commented Dec 22, 2022

kenjis commented Dec 22, 2022

MGatner left a comment

michalsn commented Dec 22, 2022

MGatner commented Dec 22, 2022

kenjis commented Dec 23, 2022

kenjis commented Dec 25, 2022 •

edited

Loading

michalsn commented Dec 25, 2022

MGatner commented Dec 26, 2022

feat: add IncomingRequest::is() method #6995

feat: add IncomingRequest::is() method #6995

Conversation

kenjis commented Dec 20, 2022 • edited Loading

datamweb left a comment

Choose a reason for hiding this comment

michalsn Dec 20, 2022

Choose a reason for hiding this comment

kenjis Dec 20, 2022

Choose a reason for hiding this comment

kenjis Dec 20, 2022

Choose a reason for hiding this comment

iRedds Dec 20, 2022

Choose a reason for hiding this comment

michalsn Dec 20, 2022

Choose a reason for hiding this comment

iRedds commented Dec 20, 2022

kenjis commented Dec 20, 2022

iRedds commented Dec 20, 2022

MGatner commented Dec 20, 2022

michalsn commented Dec 20, 2022

kenjis commented Dec 20, 2022 • edited Loading

kenjis commented Dec 21, 2022

MGatner commented Dec 21, 2022

kenjis commented Dec 22, 2022

michalsn commented Dec 22, 2022

kenjis commented Dec 22, 2022

kenjis commented Dec 22, 2022

MGatner left a comment

Choose a reason for hiding this comment

michalsn commented Dec 22, 2022

MGatner commented Dec 22, 2022

kenjis commented Dec 23, 2022

kenjis commented Dec 25, 2022 • edited Loading

michalsn commented Dec 25, 2022

MGatner commented Dec 26, 2022

kenjis commented Dec 20, 2022 •

edited

Loading

kenjis commented Dec 20, 2022 •

edited

Loading

kenjis commented Dec 25, 2022 •

edited

Loading