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

fix: AssertionError occurs when using Validation in CLI #6452

Merged
merged 4 commits into from
Aug 30, 2022

Conversation

daycry
Copy link
Contributor

@daycry daycry commented Aug 29, 2022

Description

at SYSTEMPATH\Validation\FileRules.php:42

Backtrace:
1 [internal function]
CodeIgniter\Debug\Exceptions()->errorHandler(2, 'assert(): assert($request instanceof IncomingRequest) failed', 'C:\laragon\www\test\vendor\codeigniter4\framework\system\Validation\FileRules.php', 42, [...])

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

at SYSTEMPATH\Validation\FileRules.php:42

Backtrace:
  1    [internal function]
       CodeIgniter\Debug\Exceptions()->errorHandler(2, 'assert(): assert($request instanceof IncomingRequest) failed', 'C:\\laragon\\www\\test\\vendor\\codeigniter4\\framework\\system\\Validation\\FileRules.php', 42, [...])
@NSURLSession0
Copy link
Contributor

I aso ran into this issue when executing php spark db:seed <seeder> from the terminal. Thank you for the PR!

Backtrace
[AssertionError]

assert($request instanceof IncomingRequest)

at SYSTEMPATH/Validation/FileRules.php:42

Backtrace:
  1    SYSTEMPATH/Validation/FileRules.php:42
       assert(false, 'assert($request instanceof IncomingRequest)')

  2    SYSTEMPATH/Validation/Validation.php:558
       CodeIgniter\Validation\FileRules()->__construct()

  3    SYSTEMPATH/Validation/Validation.php:122
       CodeIgniter\Validation\Validation()->loadRuleSets()

  4    SYSTEMPATH/CLI/CLI.php:324
       CodeIgniter\Validation\Validation()->run([...])

  5    SYSTEMPATH/CLI/CLI.php:257
       CodeIgniter\CLI\CLI::validate('temp', 'n', [...])

  6    APPPATH/Database/Seeds/MySeeder.php:22
       .....

@daycry
Copy link
Contributor Author

daycry commented Aug 29, 2022

I created a pull request for solve this issue.

Thank you!

@daycry
Copy link
Contributor Author

daycry commented Aug 29, 2022

Do I need anything for merge this pull un develop branch?

@iRedds
Copy link
Collaborator

iRedds commented Aug 29, 2022

@mauricevb What version are you using?
4.2.5 can not reproduce.

@kenjis kenjis changed the title - FIX: in command prompt fix: assert($request instanceof IncomingRequest) failed in Validation\FileRules.php Aug 29, 2022
@iRedds
Copy link
Collaborator

iRedds commented Aug 29, 2022

@daycry You need a normal description of the problem and tests, because it is not clear what behavior your PR corrects.

@kenjis
Copy link
Member

kenjis commented Aug 29, 2022

@daycry Thank you for trying to improve CI4!

You need to:

Copy link
Collaborator

@ddevsr ddevsr left a comment

Choose a reason for hiding this comment

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

If you use validation for CLI, now can use IncomingRequest only.

You must set assert($request instanceof CLIRequest ?? IncomingRequest), and now can use for CLI and Web

@@ -39,7 +40,7 @@ public function __construct(?RequestInterface $request = null)
$request = Services::request();
}

assert($request instanceof IncomingRequest);
assert($request instanceof CLIRequest ?? IncomingRequest)
Copy link
Member

Choose a reason for hiding this comment

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

This is not nullable, how would this work?

@daycry
Copy link
Contributor Author

daycry commented Aug 30, 2022

You can reproduce the error with this code for example

CLI::prompt('Config file already exists, do you want to replace it?', [ 'y', 'n' ]) == 'n')

When you try to validate a input with cli commands, you can see the error.

@kenjis
Copy link
Member

kenjis commented Aug 30, 2022

CLI::prompt('Config file already exists, do you want to replace it?', [ 'y', 'n' ]) == 'n')

I got:

[ParseError]

Unclosed '{' on line 10 does not match ')'

@kenjis
Copy link
Member

kenjis commented Aug 30, 2022

I was able to reproduce.

<?php

namespace App\Controllers;

use CodeIgniter\CLI\CLI;

class Home extends BaseController
{
    public function index()
    {
        $email = CLI::prompt('What is your email?', null, 'required|valid_email');
    }
}
$ php public/index.php 
What is your email? : 


[AssertionError]

assert($request instanceof IncomingRequest)

at SYSTEMPATH/Validation/FileRules.php:42

@kenjis kenjis changed the title fix: assert($request instanceof IncomingRequest) failed in Validation\FileRules.php fix: AssertionError occurs when using Validation in CLI Aug 30, 2022
@kenjis
Copy link
Member

kenjis commented Aug 30, 2022

Validation::loadRuleSets() creates all validation rule instances when Validation::run() is called.

protected function loadRuleSets()
{
if (empty($this->ruleSetFiles)) {
throw ValidationException::forNoRuleSets();
}
foreach ($this->ruleSetFiles as $file) {
$this->ruleSetInstances[] = new $file();
}
}

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Aug 30, 2022
@daycry
Copy link
Contributor Author

daycry commented Aug 30, 2022

I use this code for copy Config file in Config namespace.

if (file_exists($appPath . $path) && CLI::prompt('Config file already exists, do you want to replace it?', [ 'y', 'n' ]) == 'n') {
CLI::error('Cancelled');
exit();
}
.

[ErrorException]

assert(): assert($request instanceof IncomingRequest) failed

at SYSTEMPATH\Validation\FileRules.php:42

Backtrace:
1 [internal function]
CodeIgniter\Debug\Exceptions()->errorHandler(2, 'assert(): assert($request instanceof IncomingRequest) failed', 'C:\laragon\www\test\vendor\codeigniter4\framework\system\Validation\FileRules.php', 42, [...])
This code return this error

My first PR solve this error, because $request are CliRequest no IncomingRequest class

assert($request instanceof IncomingRequest); -->assert($request instanceof Request);

Because extends from Request, but phpstan check return error.

Later, I try this change

assert($request instanceof IncomingRequest || $request instanceof CLIRequest)

but docker postgre return and error, I think that this error isn't from the code, later will tray again.

Thank you!

@ddevsr
Copy link
Collaborator

ddevsr commented Aug 30, 2022

Validation::loadRuleSets() creates all validation rule instances when Validation::run() is called.

protected function loadRuleSets()
{
if (empty($this->ruleSetFiles)) {
throw ValidationException::forNoRuleSets();
}
foreach ($this->ruleSetFiles as $file) {
$this->ruleSetInstances[] = new $file();
}
}

@kenjis In prompt CLI by default running validate if parameter $validation not null

while (! static::validate(trim($field), $input, $validation)) {

protected static function validate(string $field, string $value, $rules): bool
{
$label = $field;
$field = 'temp';
$validation = Services::validation(null, false);
$validation->setRules([
$field => [
'label' => $label,
'rules' => $rules,
],
]);
$validation->run([$field => $value]);
if ($validation->hasError($field)) {
static::error($validation->getError($field));
return false;
}
return true;
}

@ddevsr
Copy link
Collaborator

ddevsr commented Aug 30, 2022

I already test this PR, fix validation in CLI.

image

@kenjis
Copy link
Member

kenjis commented Aug 30, 2022

but docker postgre return and error, I think that this error isn't from the code, later will tray again.

Do you mean the GitHub Action check?
I re-run the failed job.

@kenjis
Copy link
Member

kenjis commented Aug 30, 2022

I already test this PR, fix validation in CLI.

Yes, this works.

$ php public/index.php 
What is your email? : 
The What is your email? field is required.
What is your email? : 

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Specifying Request types was @kenjis' work so if he's good with this then it is fine by me. Thank you all for the discussion, and @daycry for the fix.

@kenjis
Copy link
Member

kenjis commented Aug 30, 2022

The Validation FileRules only works with Uploaded Files (HTTP). So in CLI it does not work.
Therefore the request object in it should be IncommingRequest.

But Validation::run() instantiate all the rule object, so FileRules is also instantiated even if in CLI.
And it causes AssertionError.

This PR is workaround.

@kenjis
Copy link
Member

kenjis commented Sep 1, 2022

This issue is more serious than what I thought first.

$ php spark shield:setup

CodeIgniter v4.2.5 Command Line Tool - Server Time: 2022-09-01 11:41:17 UTC+09:00

  Created: APPPATH/Config/Auth.php
  Created: APPPATH/Config/AuthGroups.php
  Updated: APPPATH/Controllers/BaseController.php
  Updated: APPPATH/Config/Routes.php
  Updated: We have updated file 'APPPATH/Config/Security.php' for security reasons.
  Run `spark migrate --all` now? [y, n]: y

[AssertionError]

assert($request instanceof IncomingRequest)

at SYSTEMPATH/Validation/FileRules.php:42

Backtrace:
  1    SYSTEMPATH/Validation/FileRules.php:42
       assert(false, 'assert($request instanceof IncomingRequest)')

  2    SYSTEMPATH/Validation/Validation.php:558
       CodeIgniter\Validation\FileRules()->__construct()

  3    SYSTEMPATH/Validation/Validation.php:122
       CodeIgniter\Validation\Validation()->loadRuleSets()

  4    SYSTEMPATH/CLI/CLI.php:324
       CodeIgniter\Validation\Validation()->run([...])

  5    SYSTEMPATH/CLI/CLI.php:257
       CodeIgniter\CLI\CLI::validate('temp', 'y', [...])

  6    VENDORPATH/codeigniter4/shield/src/Commands/Setup.php:316
       CodeIgniter\CLI\CLI::prompt('  Run `spark migrate --all` now?', [...])

  7    VENDORPATH/codeigniter4/shield/src/Commands/Setup.php:302
       CodeIgniter\Shield\Commands\Setup()->cliPrompt('  Run `spark migrate --all` now?', [...])

  8    VENDORPATH/codeigniter4/shield/src/Commands/Setup.php:92
       CodeIgniter\Shield\Commands\Setup()->runMigrations()

  9    VENDORPATH/codeigniter4/shield/src/Commands/Setup.php:79
       CodeIgniter\Shield\Commands\Setup()->publishConfig()

 10    SYSTEMPATH/CLI/Commands.php:63
       CodeIgniter\Shield\Commands\Setup()->run([])

 11    SYSTEMPATH/CLI/CommandRunner.php:65
       CodeIgniter\CLI\Commands()->run('shield:setup', [])

 12    SYSTEMPATH/CLI/CommandRunner.php:51
       CodeIgniter\CLI\CommandRunner()->index([])

 13    SYSTEMPATH/CodeIgniter.php:897
       CodeIgniter\CLI\CommandRunner()->_remap('index', [...])

 14    SYSTEMPATH/CodeIgniter.php:457
       CodeIgniter\CodeIgniter()->runController(Object(CodeIgniter\CLI\CommandRunner))

 15    SYSTEMPATH/CodeIgniter.php:336
       CodeIgniter\CodeIgniter()->handleRequest(null, Object(Config\Cache), false)

 16    SYSTEMPATH/CLI/Console.php:48
       CodeIgniter\CodeIgniter()->run()

 17    ROOTPATH/spark:98
       CodeIgniter\CLI\Console()->run()

@jozefrebjak
Copy link
Contributor

This issue is more serious than what I thought first.

$ php spark shield:setup

CodeIgniter v4.2.5 Command Line Tool - Server Time: 2022-09-01 11:41:17 UTC+09:00

  Created: APPPATH/Config/Auth.php
  Created: APPPATH/Config/AuthGroups.php
  Updated: APPPATH/Controllers/BaseController.php
  Updated: APPPATH/Config/Routes.php
  Updated: We have updated file 'APPPATH/Config/Security.php' for security reasons.
  Run `spark migrate --all` now? [y, n]: y

[AssertionError]

assert($request instanceof IncomingRequest)

at SYSTEMPATH/Validation/FileRules.php:42

Backtrace:
  1    SYSTEMPATH/Validation/FileRules.php:42
       assert(false, 'assert($request instanceof IncomingRequest)')

  2    SYSTEMPATH/Validation/Validation.php:558
       CodeIgniter\Validation\FileRules()->__construct()

  3    SYSTEMPATH/Validation/Validation.php:122
       CodeIgniter\Validation\Validation()->loadRuleSets()

  4    SYSTEMPATH/CLI/CLI.php:324
       CodeIgniter\Validation\Validation()->run([...])

  5    SYSTEMPATH/CLI/CLI.php:257
       CodeIgniter\CLI\CLI::validate('temp', 'y', [...])

  6    VENDORPATH/codeigniter4/shield/src/Commands/Setup.php:316
       CodeIgniter\CLI\CLI::prompt('  Run `spark migrate --all` now?', [...])

  7    VENDORPATH/codeigniter4/shield/src/Commands/Setup.php:302
       CodeIgniter\Shield\Commands\Setup()->cliPrompt('  Run `spark migrate --all` now?', [...])

  8    VENDORPATH/codeigniter4/shield/src/Commands/Setup.php:92
       CodeIgniter\Shield\Commands\Setup()->runMigrations()

  9    VENDORPATH/codeigniter4/shield/src/Commands/Setup.php:79
       CodeIgniter\Shield\Commands\Setup()->publishConfig()

 10    SYSTEMPATH/CLI/Commands.php:63
       CodeIgniter\Shield\Commands\Setup()->run([])

 11    SYSTEMPATH/CLI/CommandRunner.php:65
       CodeIgniter\CLI\Commands()->run('shield:setup', [])

 12    SYSTEMPATH/CLI/CommandRunner.php:51
       CodeIgniter\CLI\CommandRunner()->index([])

 13    SYSTEMPATH/CodeIgniter.php:897
       CodeIgniter\CLI\CommandRunner()->_remap('index', [...])

 14    SYSTEMPATH/CodeIgniter.php:457
       CodeIgniter\CodeIgniter()->runController(Object(CodeIgniter\CLI\CommandRunner))

 15    SYSTEMPATH/CodeIgniter.php:336
       CodeIgniter\CodeIgniter()->handleRequest(null, Object(Config\Cache), false)

 16    SYSTEMPATH/CLI/Console.php:48
       CodeIgniter\CodeIgniter()->run()

 17    ROOTPATH/spark:98
       CodeIgniter\CLI\Console()->run()

I had the same problem today with fresh v4.2.4 and shield:setup command.

@datamweb
Copy link
Contributor

datamweb commented Sep 1, 2022

@kenjis and @jozefrebjak I cannot reproduce if:
CI 4.2.5 (whit applay this PR) and shield (last dev)

Screenshot 2022-09-01 111046

@kenjis
Copy link
Member

kenjis commented Sep 1, 2022

@datamweb Yes, this PR fixes the error.

@MGatner
Copy link
Member

MGatner commented Sep 1, 2022

Should we hotfix this @kenjis ?

@kenjis
Copy link
Member

kenjis commented Sep 1, 2022

@MGatner If possible, v4.2.6 should be released as soon as possible.

Now the commands that use CLI::prompt() with the validation rules or a list of options do not work at all.

@MGatner
Copy link
Member

MGatner commented Sep 2, 2022

Did this happen because we are lacking tests? Or is it because CLI and requests are hard to test under "real" scenarios? Or...?

@kenjis
Copy link
Member

kenjis commented Sep 2, 2022

Both.
First, CLI::prompt() is untestable in 4.2.x.
We don't have integration test for a CLI commnad with CLI::prompt() in 4.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants