Dev: using filter_* functions instead direct access to global variables

[WinterSilence]

Example:

$_GET['foo'] = 1;
var_export($_GET); // ['foo' => 1]
var_export(filter_input_array(INPUT_GET)); // []

As you can see filter_input_array() return initial data, it's better use in CI because users (or vendor packages) can change globals before calling CI methods.

If there are no objections, then I will replace direct access to global variables everywhere. I will write sniff to fix this problem in future too.

[kenjis]

As you can see filter_input_array() return initial data, it's better use in CI because users (or vendor packages) can change globals before calling CI methods.

filter_input_array(INPUT_GET)) returns the GET data that only in the incomming request?
If so, the change you suggrest is too restrictive for the current design of CI4,
and may break the exsisting test code.

The current test/product code is heavily depends on the suprer global variables.

[WinterSilence]

@kenjis please see manual, it's only example to display: filter_input_array() returning initial data without changes.

[kenjis]

Sorry, I know INPUT_GET is example.

If we use filter_input_array(), we can't change the data., right?
If we can't change the data, we can't write test code.

[WinterSilence]

@kenjis https://phpunit.readthedocs.io/en/9.5/fixtures.html#global-state

[kenjis]

@WinterSilence What is your point?

[WinterSilence]

@ kenjis one test = one request, change globals in test is bad practice

[kenjis]

@WinterSilence CI4 has a lot of (test/production) code that depends on super global variables.
If we add this change, will all our existing tests pass?

[WinterSilence]

@kenjis i don't check all test, but if they are written correctly, then it must be work fine.

[kenjis]

I confirmed the behavior.

<?php
namespace App\Controllers;

class Home extends BaseController
{
    public function index()
    {
        $_GET['foo'] = 1;
        d($_GET); // ['foo' => 1]
        d(filter_input_array(INPUT_GET)); // []
    }

}

[kenjis]

@WinterSilence
Will this test pass?
https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/HTTP/IncomingRequestTest.php#L48-L54

[WinterSilence]

@kenjis

but if they are written correctly, then it must be work fine.

it's not correct tests.

You can keep "pure" request data if any user or vendor library can change they. I didn't say that the right decisions are easy. And I didn't say that it's enough to replace global variables with a call filter_*().

[kenjis]

@WinterSilence

it's not correct tests.

How do you fix the test (and product code)?
https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/HTTP/IncomingRequestTest.php#L48-L54

[WinterSilence]

@kenjis https://phpunit.readthedocs.io/en/9.5/configuration.html#appendixes-configuration-php-post , mosk request class or https://blog.cloudflare.com/using-guzzle-and-phpunit-for-rest-api-testing/

[kenjis]

https://phpunit.readthedocs.io/en/9.5/configuration.html#appendixes-configuration-php-post

But super globals change per test. If we can't change them, we can't write enough test cases.

mosk request class

Do you mean mock the class to test? And how do you mock the IncomingRequest?

https://blog.cloudflare.com/using-guzzle-and-phpunit-for-rest-api-testing/

It is not unit testing. We need fast unit tests.

[WinterSilence]

@kenjis One test = one request - see @backupglobals

1. IncomingRequest must use $this->globals['...'] instead direct access to $_...

   CodeIgniter4/system/HTTP/RequestTrait.php

   Line 306 in 6550690

   protected function populateGlobals(string $method)

   pass global variables by $config into constructor and initialize $this->globals
2. IncomingRequest use getenv('QUERY_STRING') - you can call setenv('QUERY_STRING', '...')

It is not unit testing.

What do you think this is?

[kenjis]

I think injecting the superglobals data into IncomingRequest is the way to go.
2. is not good. It is essentially the same as using superglobals in the IncomingRequest.
Superglobals or environment variables. And QUERY_STRING is only for $_GET.
What about other data?

By the way, what do you want to achieve with this filter_* function?
For example, Services::request() returns the IncomingRequest like this.
Will your goal be achieved?

$request->setGlobal('get', filter_input_array(INPUT_GET));

[kenjis]

No feedback. So I close.

I'm still not sure this should be implemented or not, but before implementing this we need to rework all classes that handles the super globals.
These classes should be injected super global values as constructor parameters, not using super globals inside.

[WinterSilence]

@kenjis sorry, miss your answer

By the way, what do you want to achieve with this filter_* function?

I want keep initial data of request. Now request data can be changed by vendor/user code, framework must ignore this changes. If developer wants to make changes, then he must do so by explicitly changing the corresponding method or property.