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: BaseModel::insert() may not pass all the values from Entity #4980

Merged
merged 8 commits into from
Nov 23, 2021

Conversation

katie1348
Copy link
Contributor

@katie1348 katie1348 commented Jul 30, 2021

Fixes #4247

Updated the parameter passed, so the function behaves differently for Insert and Update, aligned with what would be expected.

Description
Added in a test to use all the fields when Inserting and to do a changed fields when Updating. The old version always only inserted changed fields, which is not what is expected.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Applied the fix that enables the INSERT to work correctly when inserting a row that already contains data.
@katie1348 katie1348 changed the title Katie1348 patch 2 Fix for Issue #4247 Jul 30, 2021
@paulbalandan paulbalandan changed the title Fix for Issue #4247 Update of BaseModel.php to fix INSERT issue Jul 30, 2021
@paulbalandan paulbalandan added the tests needed Pull requests that need tests label Jul 31, 2021
@lonnieezell
Copy link
Member

@katie1348 Your description says you added in a test but I don't see any in the PR. Are you able to add that?

@MichalPB1
Copy link

What is the status of? It also resolves my problem

@paulbalandan
Copy link
Member

@MichalPB1 if you can help add a test for this change then it is mergeable.

@katie1348
Copy link
Contributor Author

I would write a test, but I don't know how.

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 11, 2021
@kenjis kenjis changed the title Update of BaseModel.php to fix INSERT issue fix: BaseModel::insert() may not pass all the values from Entity Nov 11, 2021
@katie1348
Copy link
Contributor Author

Hi Kenjis,

Thanks for the pointers and I have the system installed and the repo updated and cloned to my Mac.

When I run:

phpunit

I get the following error (stack trace omitted)

PHP Fatal error: Uncaught Error: Class "CodeIgniter\Models\LiveModelTestCase" not found in /Users/user/Sites/CodeIgniter4/tests/system/Models/AffectedRowsTest.php:19

I have no idea why a clean unchanged clone of the repo would throw up this error, any thoughts?

Thanks in advance.

@kenjis
Copy link
Member

kenjis commented Nov 11, 2021

@katie1348 You maybe miss something.

This is how to:

$ git clone [email protected]:<your-name>/CodeIgniter4.git
$ cd CodeIgniter4/
$ composer install
$ vendor/bin/phpunit

@katie1348
Copy link
Contributor Author

@kenjis Thank you for your help, much appreciated.

That now has it working, mostly. It fails some tests as I don't have Redis etc installed and will not. But it looks like it is up and running enough to write the necessary test.

Hopefully next week.

@kenjis
Copy link
Member

kenjis commented Nov 12, 2021

@katie1348

You can run one test file:

$ vendor/bin/phpunit tests/system/HTTP/RequestTest.php

Or exclude live tests:

$ vendor/bin/phpunit --exclude-group DatabaseLive,CacheLive

@katie1348
Copy link
Contributor Author

@kenjis

Once again, thank you for the assistance.

With your help, I have been able to get the test working and it is now ready for additional checking etc.

I am now stuck on the cs-fixer. I cannot seem to get it to run correctly. I have installed it with homebrew and it looks like it is working sometimes, but it does not seem to change the files.

When run from the command line, it get an error:

Fatal error: Uncaught Error: Class "Nexus\CsConfig\FixerGenerator" not found in /Users/user/Sites/CodeIgniter4/.php-cs-fixer.dist.php:52

I am once again stuck, your assistance would be appreciated.

@kenjis
Copy link
Member

kenjis commented Nov 16, 2021

@katie1348 See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#php-style

I have installed it with homebrew and it looks like it is working sometimes, but it does not seem to change the files.

This is a common mistake. We use php-cs-fixer for this project, and you have already installed it
via Composer.

And you can run with composer.

$ composer cs-fix

User.php is an Entity
UserObjModel.php returns an entity not StdObj
@katie1348
Copy link
Contributor Author

@kenjis Thank you for that assistance. It got me over the hump.

OK, I have uploaded the changes to my fork, in the correct branch.

How do I go about getting the 6 Workflows awaiting approval to be run?

@kenjis
Copy link
Member

kenjis commented Nov 16, 2021

How do I go about getting the 6 Workflows awaiting approval to be run?

You can't do anything. I approved.

@katie1348
Copy link
Contributor Author

Updated to fix Reactor failures.

@katie1348
Copy link
Contributor Author

@kenjis

Thanks for all your help in getting this done. I don't think I could have done it without you.

Is there anything else I need to do?

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

@katie1348 I added review comments.
Please fix your test code.

If you have any questions, feel free to ask.

@kenjis kenjis added database Issues or pull requests that affect the database layer and removed tests needed Pull requests that need tests labels Nov 17, 2021
@katie1348
Copy link
Contributor Author

Requested changes made.

@katie1348
Copy link
Contributor Author

@kenjis I have accepted your changes to the tests.

@katie1348 katie1348 requested a review from kenjis November 19, 2021 14:20
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

LGTM

@katie1348
Copy link
Contributor Author

@kenjis Thanks for all your help and patience.

@paulbalandan
Copy link
Member

Also, please squash before merging to remove the merge commit.

@kenjis kenjis merged commit b8fe16d into codeigniter4:develop Nov 23, 2021
@kenjis
Copy link
Member

kenjis commented Nov 23, 2021

@katie1348 Congratulations! You are now a contributor to this project. 🎉

@MGatner
Copy link
Member

MGatner commented Nov 23, 2021

Thanks all for the work! Sorry this one sat so long, looks like a quality PR - just slipped through the cracks.

@katie1348
Copy link
Contributor Author

@katie1348 Congratulations! You are now a contributor to this project. 🎉

Only because you did all the hand holding I needed. Thank you once again.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: onlyChanged is currently true for INSERT
6 participants