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

feat: add 'DBGroup' for customization db group #744

Merged
merged 14 commits into from
May 22, 2023

Conversation

arashsaffari
Copy link
Contributor

@arashsaffari arashsaffari commented May 16, 2023

Supersedes #736
Ref: #727

@kenjis kenjis added new feature PRs for new features tests needed Pull requests that need tests labels May 16, 2023
@datamweb
Copy link
Collaborator

datamweb commented May 16, 2023

In three cases, how does method validateData() connect to the database with which DBGroup?

if (! $this->validateData($this->request->getPost(), $rules)) {

if (! $this->validateData($this->request->getPost(), $rules)) {

if (! $this->validateData($this->request->getPost(), $rules)) {

Git Note:
If you need to change a new file, you can do it as follows:

git switch customization_db_group
code .

Make the necessary changes for each file. (e.g. src/Controllers/RegisterController.php) and save.

composer cs-fix
git add src/Controllers/RegisterController.php
git commit -m "add `DBGroup` for `validateData()` in RegisterController"
git push origin customization_db_group

@kenjis
Copy link
Member

kenjis commented May 17, 2023

Set the fourth parameter:

protected function validateData(array $data, $rules, array $messages = [], ?string $dbGroup = null): bool

https://github.com/codeigniter4/CodeIgniter4/blob/38840b046af7eb0e5a9de8de61c82136916ee17d/system/Controller.php#L151

@datamweb
Copy link
Collaborator

The php unit test failed, this is due to wrong setting of parameters.

phpunittest

More info:
https://github.com/codeigniter4/shield/actions/runs/5005919016/jobs/8971231182?pr=744

Try to fix it:

$this->validateData($this->request->getPost(), $rules, [], config('Auth')->DBGroup);

Please also apply this change in the two files below :

https://github.com/codeigniter4/shield/blob/develop/docs/addons/jwt.md
https://github.com/codeigniter4/shield/blob/develop/docs/guides/mobile_apps.md

@arashsaffari
Copy link
Contributor Author

I have completed that. I'm sorry for producing so much effort for you. This type of collaboration is new to me.

@datamweb
Copy link
Collaborator

I'm sorry for producing so much effort for you. This type of collaboration is new to me.

It's okay, we've all been in your situation at some point.

Well, we recently made some changes to file src/Database/Migrations/2020-12-28-223112_create_auth_tables.php, so you'll need to update your branch (customization_db_group) before continuing. This is very important.

According to the sensitivity, take a backup of branch (customization_db_group) with the following command:

git switch develop
git branch -m customization_db_group customization_db_group.backup1
git switch -c customization_db_group  customization_db_group.backup1

Then, update branch is explained here:

git fetch upstream
git switch develop
git merge upstream/develop
git push origin develop
git switch customization_db_group
git rebase upstream/develop
code .

More info here

After that, you should add this feature to the src/Database/Migrations/2020-12-28-223112_create_auth_tables.php file:

public function __construct(?Forge $forge = null)
{
-parent::__construct($forge);

-/** @var Auth $authConfig */
-$authConfig       = config('Auth');
-$this->tables     = $authConfig->tables;
-$this->attributes = ($this->db->getPlatform() === 'MySQLi') ? ['ENGINE' => 'InnoDB'] : [];
}

+/** @var Auth $authConfig */
+$authConfig       = config('Auth');
+$this->tables     = $authConfig->tables;
+$this->attributes = ($this->db->getPlatform() === 'MySQLi') ? ['ENGINE' => 'InnoDB'] : [];

+$this->DBGroup = $authConfig->DBGroup; 
+parent::__construct($forge);
git add src/Database/Migrations/2020-12-28-223112_create_auth_tables.php
git commit -m "Add `DBGroup` to migration file"

And finally, run the following command

git push --force-with-lease origin customization_db_group

@kenjis
Copy link
Member

kenjis commented May 18, 2023

The migration file does not work.
Because at the point $this->db is null.

There were 213 errors:

1) Tests\Authorization\GroupTest::testRemovePermission
Error: Call to a member function getPlatform() on null

/home/runner/work/shield/shield/src/Database/Migrations/2020-12-28-223112_create_auth_tables.php:25
/home/runner/work/shield/shield/vendor/codeigniter4/framework/system/Database/MigrationRunner.php:846
/home/runner/work/shield/shield/vendor/codeigniter4/framework/system/Database/MigrationRunner.php:188
/home/runner/work/shield/shield/vendor/codeigniter4/framework/system/Test/DatabaseTestTrait.php:151
/home/runner/work/shield/shield/vendor/codeigniter4/framework/system/Test/DatabaseTestTrait.php:109
/home/runner/work/shield/shield/vendor/codeigniter4/framework/system/Test/DatabaseTestTrait.php:55
/home/runner/work/shield/shield/vendor/codeigniter4/framework/system/Test/CIUnitTestCase.php:248
/home/runner/work/shield/shield/tests/_support/TestCase.php:21
/home/runner/work/shield/shield/tests/Authorization/GroupTest.php:24

https://github.com/codeigniter4/shield/actions/runs/5008438682/jobs/8977862162?pr=744

src/Models/BaseModel.php Show resolved Hide resolved
src/Models/BaseModel.php Outdated Show resolved Hide resolved
src/Models/BaseModel.php Outdated Show resolved Hide resolved
arashsaffari and others added 4 commits May 18, 2023 23:53
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
@datamweb
Copy link
Collaborator

For fix PHPCSFixer error:
All code must conform to our Style Guide, which is based on PSR-12.

This makes certain that all submitted code is of the same format as the existing code and ensures that the codebase will be as readable as possible.

You can fix most of the coding style violations by running this command in your terminal:

composer cs-fix

more info see: https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#php-style

@datamweb datamweb requested a review from kenjis May 18, 2023 22:52
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!

@kenjis
Copy link
Member

kenjis commented May 20, 2023

@arashsaffari No, you moved it to the wrong place. Tests failed again.

1) Tests\Controllers\LoginTest::testAfterLoggedInNotDisplayLoginPage
Error: Call to a member function getPlatform() on null 

https://github.com/codeigniter4/shield/actions/runs/5029268480/jobs/9020753874?pr=744

src/Config/Auth.php Outdated Show resolved Hide resolved
@datamweb
Copy link
Collaborator

@arashsaffari for merge all PRs must include phpunit test.

Due to the specific circumstances of this PR, the team decided to merge without unittest.
If you are interested to learn how to write a phpunit test, check the link below.

https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

Thanks for the PR.

@datamweb datamweb merged commit 3dc2154 into codeigniter4:develop May 22, 2023
24 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature PRs for new features tests needed Pull requests that need tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants