docs: fix PHPDoc types in `Filter` class. #7451

ping-yee · 2023-04-25T09:43:45Z

Description
See #6310

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

ping-yee · 2023-04-25T10:01:11Z

I don't change the return type in run() function in Filters.php and two functions of FilterInterface.php because I am not sure that Is there any possibility that these methods will param/return types other than object|null|void?

MGatner · 2023-04-26T10:35:02Z

I think the filter return values need to remain mixed because the docs specify they can return any non-empty value to short circuit.

https://codeigniter4.github.io/CodeIgniter4/incoming/filters.html#creating-a-filter

ping-yee · 2023-04-26T12:21:59Z

kenjis · 2023-04-26T23:15:36Z

I don't think a filter returns resource.

system/Filters/Filters.php

kenjis · 2023-04-26T23:44:51Z

Personally, I would like to reduce the number of allowed types like the following.
I think for stopping filters only non-empty string is sufficient. But it is a breaking change.

--- a/system/Filters/FilterInterface.php
+++ b/system/Filters/FilterInterface.php
@@ -31,7 +31,7 @@ interface FilterInterface
      *
      * @param array|null $arguments
      *
-     * @return mixed
+     * @return RequestInterface|ResponseInterface|string|void
      */
     public function before(RequestInterface $request, $arguments = null);
 
@@ -43,7 +43,7 @@ interface FilterInterface
      *
      * @param array|null $arguments
      *
-     * @return mixed
+     * @return ResponseInterface|void
      */
     public function after(RequestInterface $request, ResponseInterface $response, $arguments = null);
 }

ping-yee · 2023-04-27T07:22:28Z

I think for stopping filters only non-empty string is sufficient. But it is a breaking change.

I changed the PHPDoc return type first, then I will open another PR to handle this.

ping-yee · 2023-04-27T12:14:36Z

@kenjis I want to check what will need to be change in Filter class if we want to stopping filters only non-empty string.

kenjis · 2023-04-27T12:32:03Z

If we change the code, It is a breaking change.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#breaking-changes
So I think it is better to make non-empty values except string deprecated (stating that in the user guide).
Then in the next or next next minor versions we permit only non-empty string.

ping-yee · 2023-04-27T15:20:18Z

Then in the next or next next minor versions we permit only non-empty string.

I am willing to help with this BC feature in the future!

So I think it is better to make non-empty values except string deprecated (stating that in the user guide).

So do I need to modify the user guide in this PR? Or need to another PR to handle this pre-work.

And I wonder if there is anything else that needs to be modified in this PR.

kenjis · 2023-04-30T02:13:05Z

system/Filters/FilterInterface.php

@@ -31,7 +31,7 @@ interface FilterInterface
     *
     * @param array|null $arguments
     *
-     * @return mixed
+     * @return RequestInterface|ResponseInterface|string|void


I'm okay with this change, but I would not mind if we left it as it was (only mixed).
I don't have a strong opinion.

kenjis · 2023-04-30T02:21:52Z

I agree with the future change to only allow non-empty strings, but I don't know if the team would agree to that.

@codeigniter4/core-team Any comment?

michalsn

If we want to introduce these return-type changes, can we do it once, without breaking it down into further releases? Are we afraid that this would break people's code entirely?

I was thinking about a scenario where this could break the app for good, and I couldn't find anything.

michalsn · 2023-04-30T15:16:11Z

system/Filters/FilterInterface.php

@@ -31,7 +31,7 @@ interface FilterInterface
     *
     * @param array|null $arguments
     *
-     * @return mixed
+     * @return RequestInterface|ResponseInterface|string|void


I'm okay with this change, but I would not mind if we left it as it was (only mixed).
I don't have a strong opinion.

ping-yee · 2023-05-01T08:38:20Z

@kenjis @michalsn @MGatner I have been change the return type mixed in FilterInterface, review please.

I agree with the future change to only allow non-empty strings.

I will open the other PR to handle this after this PR is merged.

If we want to introduce these return-type changes, can we do it once, without breaking it down into further releases? Are we afraid that this would break people's code entirely?

I think the better way is add a notice at user guide page first and may be update non-empty string in next version as Kenjis says.

kenjis · 2023-05-01T12:13:57Z

@michalsn Changing the return type means changing the if condition:

CodeIgniter4/system/Filters/Filters.php

Lines 187 to 192 in 729634c

    
           // Ignore an empty result 
        
           if (empty($result)) { 
        
               continue; 
        
           } 
        
           return $result;

This is a breaking change.

michalsn · 2023-05-01T13:27:02Z

@kenjis If we want to change the return type, we can't avoid a breaking change.
I would just target this to 4.4 and that's it.

Adding a note to the user guide first and then changing it in the future gives us nothing. Developers who already have built their apps most likely won't see the note. They will notice it when we introduce a change into the code - hopefully reading the release notes.

The real question is if other developers take advantage of the current implementation. Because for me, that's the only real problem.

kenjis · 2023-05-02T01:02:36Z

The real question is if other developers take advantage of the current implementation. Because for me, that's the only real problem.

I don't know. But it is documented as non-empty value, some devs may be using true or something. I don't think many devs actually use this feature.

It is possible that this change could actually break existing applications. So I don't strongly advocate making the change.

However, I don't think there is a need to allow any non-empty value. We don't even use the return value itself, so if we want to stop later filters, I think one type value for it is sufficient.

ping-yee · 2023-05-08T18:57:42Z

So... Is there a consensus here?

@michalsn @kenjis @MGatner

Or can we have a notice for user guide first and change return type to non-empty string in 4.4?

I think have a notice in user guide may not make the developers who have been implemented to make changes, but it can prevent future devs who need this method from defining other types.

michalsn · 2023-05-08T19:19:14Z

The current changes seem fine.

I don't see a good reason to change the return signature for FilterInterface and introduce a BC break for that reason.

I'm okay with adding a note to the user guide to return only non-empty strings.

kenjis · 2023-05-09T03:58:07Z

This PR is okay.

iRedds

Considering that PSR-15 was already 5 years ago, the use of the approach applied in CI surprises me a little.
In addition, the implementation of filter processing directly contradicts the description of filter methods.

iRedds · 2023-05-09T03:38:46Z

system/Filters/FilterInterface.php

@@ -31,7 +31,7 @@ interface FilterInterface
     *
     * @param array|null $arguments
     *
-     * @return RequestInterface|ResponseInterface|string|void
+     * @return mixed


I think the return type should be like this. Which corresponds to the description of the method.
That is, either we complete the processing of the request by returning a response. Or further processing of the request continues and the method returns nothing.

Suggested change

* @return mixed

* @return ResponseInterface|void

How to stop the later filters?

I did not understand the question.

It will return other types that are used for stop later filters.

https://codeigniter4.github.io/CodeIgniter4/incoming/filters.html#before-filters

Any filter that returns a non-empty result dequeues filters that, for example, are responsible for access.

I think this is a stupid idea.

But if such an implementation is needed (to shoot yourself in the foot), then you can throw a FilterStopPropagation exception.

iRedds · 2023-05-09T03:48:18Z

system/Filters/FilterInterface.php

@@ -43,7 +43,7 @@ public function before(RequestInterface $request, $arguments = null);
     *
     * @param array|null $arguments
     *
-     * @return ResponseInterface|void
+     * @return mixed


I think that it is more acceptable to return only ResponeInterface here.

Suggested change

* @return mixed

* @return ResponseInterface

docs: replace mixed in filter class.

a477400

MGatner approved these changes Apr 26, 2023

View reviewed changes

kenjis reviewed Apr 26, 2023

View reviewed changes

system/Filters/Filters.php Outdated Show resolved Hide resolved

docs: replace type.

d9313c4

kenjis reviewed Apr 30, 2023

View reviewed changes

michalsn reviewed Apr 30, 2023

View reviewed changes

docs: change back mixed return type in filterInterface class.

831a401

kenjis approved these changes May 1, 2023

View reviewed changes

kenjis merged commit 42b174f into codeigniter4:develop May 9, 2023

iRedds reviewed May 9, 2023

View reviewed changes

ping-yee deleted the docs-replace-filter branch June 13, 2023 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: fix PHPDoc types in `Filter` class. #7451

docs: fix PHPDoc types in `Filter` class. #7451

ping-yee commented Apr 25, 2023

ping-yee commented Apr 25, 2023 •

edited

Loading

MGatner commented Apr 26, 2023

ping-yee commented Apr 26, 2023 •

edited

Loading

kenjis commented Apr 26, 2023

kenjis commented Apr 26, 2023

ping-yee commented Apr 27, 2023

ping-yee commented Apr 27, 2023

kenjis commented Apr 27, 2023

ping-yee commented Apr 27, 2023

kenjis Apr 30, 2023 •

edited

Loading

michalsn Apr 30, 2023

kenjis commented Apr 30, 2023

michalsn left a comment

michalsn Apr 30, 2023

ping-yee commented May 1, 2023 •

edited

Loading

kenjis commented May 1, 2023

michalsn commented May 1, 2023

kenjis commented May 2, 2023

ping-yee commented May 8, 2023 •

edited

Loading

michalsn commented May 8, 2023

kenjis commented May 9, 2023

iRedds left a comment

iRedds May 9, 2023

kenjis May 9, 2023

iRedds May 9, 2023

ping-yee May 9, 2023 •

edited

Loading

iRedds May 9, 2023

iRedds May 9, 2023

	* @return RequestInterface\|ResponseInterface\|string\|void
	* @return mixed\|RequestInterface\|ResponseInterface\|void

docs: fix PHPDoc types in Filter class. #7451

docs: fix PHPDoc types in Filter class. #7451

Conversation

ping-yee commented Apr 25, 2023

ping-yee commented Apr 25, 2023 • edited Loading

MGatner commented Apr 26, 2023

ping-yee commented Apr 26, 2023 • edited Loading

kenjis commented Apr 26, 2023

kenjis commented Apr 26, 2023

ping-yee commented Apr 27, 2023

ping-yee commented Apr 27, 2023

kenjis commented Apr 27, 2023

ping-yee commented Apr 27, 2023

kenjis Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

michalsn Apr 30, 2023

Choose a reason for hiding this comment

kenjis commented Apr 30, 2023

michalsn left a comment

Choose a reason for hiding this comment

michalsn Apr 30, 2023

Choose a reason for hiding this comment

ping-yee commented May 1, 2023 • edited Loading

kenjis commented May 1, 2023

michalsn commented May 1, 2023

kenjis commented May 2, 2023

ping-yee commented May 8, 2023 • edited Loading

michalsn commented May 8, 2023

kenjis commented May 9, 2023

iRedds left a comment

Choose a reason for hiding this comment

iRedds May 9, 2023

Choose a reason for hiding this comment

kenjis May 9, 2023

Choose a reason for hiding this comment

iRedds May 9, 2023

Choose a reason for hiding this comment

ping-yee May 9, 2023 • edited Loading

Choose a reason for hiding this comment

iRedds May 9, 2023

Choose a reason for hiding this comment

iRedds May 9, 2023

Choose a reason for hiding this comment

docs: fix PHPDoc types in `Filter` class. #7451

docs: fix PHPDoc types in `Filter` class. #7451

ping-yee commented Apr 25, 2023 •

edited

Loading

ping-yee commented Apr 26, 2023 •

edited

Loading

kenjis Apr 30, 2023 •

edited

Loading

ping-yee commented May 1, 2023 •

edited

Loading

ping-yee commented May 8, 2023 •

edited

Loading

ping-yee May 9, 2023 •

edited

Loading