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: [Model] when the query ($this->first()) result returns NULL we get TypeError #8807

Closed
wants to merge 2 commits into from

Conversation

avegacms
Copy link
Contributor

@avegacms avegacms commented Apr 19, 2024

Description
Fixes #8806

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

Bug: when the query ($this->first()) result returns NULL we get TypeError
@avegacms avegacms changed the title Fix bug #8806 fix: Model.php for 4.5.1 #8806 Apr 19, 2024
@ddevsr ddevsr added the tests needed Pull requests that need tests label Apr 19, 2024
@kenjis kenjis changed the title fix: Model.php for 4.5.1 #8806 fix: [Model] when the query ($this->first()) result returns NULL we get TypeError Apr 19, 2024
@kenjis
Copy link
Member

kenjis commented Apr 19, 2024

Can you add test code?

We expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#contributions

Also, there are three places that calls convertToReturnType().
Can you fix other places?

doFind
            $row = $this->convertToReturnType($row, $returnType);
doFindAll
                $results[$i] = $this->convertToReturnType($row, $returnType);
doFirst
            $row = $this->convertToReturnType($row, $returnType);

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Apr 19, 2024
@avegacms
Copy link
Contributor Author

I confirm that there is a TypeError error in the methods: doFind and doFirst when using casts

Fix TypeError error in the methods: doFind and doFirst when using casts
@avegacms
Copy link
Contributor Author

I'm new to Codeigniter and I don't know how to do tests, please help me with this issue. In my opinion, this error is very serious and should be uploaded in the very near future in a new version.

@kenjis
Copy link
Member

kenjis commented Apr 19, 2024

PRs will not be merged unless there is test code to prove that the bug has been fixed.

The test code is a kind of documentation and should be very easy to read and understand immediately.
Therefore, it should be not difficult to write. (In reality, however, it may be impossible or very difficult to write tests for poorly designed product code.)

In this case, writing tests is easy. You only need to add tests for the cases where the results are null, referring to existing test cases.

  1. read https://github.com/codeigniter4/CodeIgniter4/tree/develop/tests#running-system-tests
  2. see https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/Models/DataConverterModelTest.php

For example, copy this method and write a new test case method:

public function testFindAsArray(): void
{
$id = $this->prepareOneRecord();
$user = $this->model->find($id);
$this->assertIsInt($user['id']);
$this->assertInstanceOf(Time::class, $user['created_at']);
$this->assertSame('John Smith', $user['name']);
// `name` is cast by custom CastBase64 handler.
$this->seeInDatabase('user', ['name' => 'Sm9obiBTbWl0aA==']);
}

First write a new test case, and run it. Then it fails (throwing TypeError).
Then fix the product code, and run the test again. If it passes, it shows the bug has been fixed.

@kenjis kenjis added the database Issues or pull requests that affect the database layer label Apr 20, 2024
@kenjis
Copy link
Member

kenjis commented May 8, 2024

I sent #8871

@kenjis
Copy link
Member

kenjis commented May 10, 2024

Closed by #8871

@kenjis kenjis closed this May 10, 2024
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 database Issues or pull requests that affect the database layer tests needed Pull requests that need tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: [Model] when the query ($this->first()) result returns NULL we get TypeError
3 participants