feat: Allow adding another database group #34

daycry · 2022-08-31T10:03:34Z

Allow adding another database group

- Allow adding another database group

kenjis · 2022-08-31T10:22:58Z

Thank you for sending a PR.

We have updated the guideline for git commit.
Please read https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#committing

kenjis · 2022-08-31T10:24:53Z

You must GPG-sign your work, certifying that you either wrote the work or otherwise have the right to pass it on to an open-source project. See Developer's Certificate of Origin.

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

daycry · 2022-08-31T11:06:27Z

I tried it, but always return an error message.

kenjis · 2022-08-31T11:13:33Z

The basic steps:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/signing.md#secure-signing

What did you do?
and what did you get? the exact error message?

daycry · 2022-08-31T11:14:21Z

Done. Key imported

daycry · 2022-08-31T17:22:53Z

Who Can review this changes? I think that are interesting changes.

composer.json

phpunit.xml.dist

src/Database/Migrations/2021-07-04-041948_CreateSettingsTable.php

kenjis · 2022-09-01T00:48:03Z

It seems there is not test code. We need it.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

MGatner · 2022-09-01T09:43:17Z

@daycry the development kit here does need to be updated but that should happen in a separate PR. I will try to get that done today, but please remove those changes from this PR in the meantime.

I will wait to review, but generally this looks like a desirable feature!

kenjis · 2022-09-01T10:05:51Z

@MGatner I sent #35

kenjis · 2022-09-01T10:17:01Z

@daycry We have updated the development kit. Please rebase to resolve conflicts and remove unneeded changes in this PR.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

daycry · 2022-09-01T11:42:22Z

Ok, I Will do It.

Although I have seen that the changes you have made in composer and phpunit are the same as I have done, I will delete my changes and create the pull request again.

thank you!

daycry · 2022-09-01T22:41:06Z

I do the signed commit.

MGatner

Looking good! Some design thoughts. Also please do try some tests - there should be some there that make a decent starting point.

MGatner · 2022-09-02T09:40:56Z

src/Config/Settings.php

 use CodeIgniter\Settings\Handlers\ArrayHandler;
 use CodeIgniter\Settings\Handlers\DatabaseHandler;

-class Settings
+class Settings extends BaseConfig


I'm not sure if there were recursion issues or something why Lonnie didn't originally make this an actual Config class. @lonnieezell you available to comment?

src/Database/Migrations/2021-07-04-041948_CreateSettingsTable.php

MGatner · 2022-09-02T09:43:32Z

src/Database/Migrations/2021-07-04-041948_CreateSettingsTable.php

@@ -2,10 +2,22 @@

 namespace CodeIgniter\Settings\Database\Migrations;

+use CodeIgniter\Config\BaseConfig;


It is okay to require the Config to be ours (or an extension thereof):

Suggested change

use CodeIgniter\Config\BaseConfig;

use CodeIgniter\Settings\Config\Settings;

MGatner · 2022-09-02T09:44:26Z

src/Database/Migrations/2021-11-14-143905_AddContextColumn.php

@@ -2,10 +2,22 @@

 namespace CodeIgniter\Settings\Database\Migrations;

+use CodeIgniter\Config\BaseConfig;


Suggested change

use CodeIgniter\Config\BaseConfig;

use CodeIgniter\Settings\Config\Settings;

MGatner · 2022-09-02T09:44:46Z

src/Database/Migrations/2021-07-04-041948_CreateSettingsTable.php

 use CodeIgniter\Database\Migration;

 class CreateSettingsTable extends Migration
 {
+    private BaseConfig $config;


Suggested change

private BaseConfig $config;

private Settings $config;

Never mind, see below: this can be dropped altogether.

MGatner · 2022-09-02T09:45:01Z

src/Database/Migrations/2021-11-14-143905_AddContextColumn.php

 use CodeIgniter\Database\Migration;

 class AddContextColumn extends Migration
 {
+    private BaseConfig $config;


Suggested change

private BaseConfig $config;

private Settings $config;

src/Database/Migrations/2021-11-14-143905_AddContextColumn.php

src/Handlers/DatabaseHandler.php

daycry · 2022-09-02T10:26:25Z

Ok, I Will review your suggestions, and I Will do some tests integrating the group attribute. Thank you.

…

On Fri, 2 Sept 2022, 11:52 MGatner, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Database/Migrations/2021-07-04-041948_CreateSettingsTable.php <#34 (comment)>: > use CodeIgniter\Database\Migration; class CreateSettingsTable extends Migration { + private BaseConfig $config; Never mind, see below: this can be dropped altogether. — Reply to this email directly, view it on GitHub <#34 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABZ5DP6Z33D6JCDHVH67BY3V4HE7RANCNFSM6AAAAAAQBG7AA4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

This may break backwards compatibility as if it doesn't check if it exists it will return error, because group doesn't currently exist. Co-authored-by: MGatner <mgatner@icloud.com>

…evelop

MGatner

The error handling is probably a case for going with BaseConnection instead of BaseBuilder, but since these are "things really went wrong" situations it is more of a debug so I'm okay with it. Let's see what @kenjis thinks.

kenjis · 2022-09-21T02:08:33Z

@MGatner What do you mean with the error handling?

kenjis · 2022-09-21T02:11:35Z

This PR looks good. I'm merging for now.

MGatner · 2022-09-21T10:20:53Z

@kenjis I meant the way the RuntimeExceptions still rely on BaseConnection instead of BaseBuilder. Either way it isn't proper adherence to Law of Demeter, but in this case the connection is only needed for error handling so isn't really a dependency. It's also an issue with our database classes, that errors are not accessible from the class where they occurred.

So yes: this is good, thanks for merging!

MGatner · 2022-09-21T10:21:56Z

(In the future though we should probably be squashing PRs like this that have "train of thought" commits.)

daycry · 2022-10-11T08:20:00Z

I tried it, but always return an error message. El mié, 31 ago 2022 a las 12:25, kenjis ***@***.***>) escribió:

…

You must GPG-sign <https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/signing.md> your work, certifying that you either wrote the work or otherwise have the right to pass it on to an open-source project. See Developer's Certificate of Origin <https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/DCO.md> . See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing — Reply to this email directly, view it on GitHub <#34 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABZ5DP3ODPAHNOMH54ZAV73V34XH7ANCNFSM6AAAAAAQBG7AA4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

- Improvements

8b0fe2c

- Allow adding another database group

kenjis reviewed Sep 1, 2022

View reviewed changes

composer.json Outdated Show resolved Hide resolved

kenjis reviewed Sep 1, 2022

View reviewed changes

phpunit.xml.dist Outdated Show resolved Hide resolved

kenjis reviewed Sep 1, 2022

View reviewed changes

src/Database/Migrations/2021-07-04-041948_CreateSettingsTable.php Outdated Show resolved Hide resolved

kenjis changed the title ~~- Improvements~~ Allow adding another database group Sep 1, 2022

kenjis changed the title ~~Allow adding another database group~~ feat: Allow adding another database group Sep 1, 2022

daycry added 2 commits September 2, 2022 00:33

Merge remote-tracking branch 'upstream/develop' into develop

36e24dd

- remove private method

8065827

MGatner requested changes Sep 2, 2022

View reviewed changes

daycry and others added 8 commits September 3, 2022 11:10

Update src/Database/Migrations/2021-11-14-143905_AddContextColumn.php

3080c6b

This may break backwards compatibility as if it doesn't check if it exists it will return error, because group doesn't currently exist. Co-authored-by: MGatner <mgatner@icloud.com>

- Testing

136734a

Merge branch 'develop' of https://github.com/daycry/settings-1 into d…

1e7ecd3

…evelop

Update DatabaseHandlerTest.php

0261135

. Refactoring

507b340

Merge branch 'develop' into develop

1d48c98

Update DatabaseHandler.php

79406ed

Merge branch 'develop' of https://github.com/daycry/settings-1 into d…

980dc30

…evelop

MGatner approved these changes Sep 5, 2022

View reviewed changes

MGatner requested a review from kenjis September 5, 2022 11:35

kenjis approved these changes Sep 21, 2022

View reviewed changes

kenjis merged commit c648852 into codeigniter4:develop Sep 21, 2022

kenjis added the enhancement New feature or request label Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow adding another database group #34

feat: Allow adding another database group #34

daycry commented Aug 31, 2022

kenjis commented Aug 31, 2022

kenjis commented Aug 31, 2022

daycry commented Aug 31, 2022

kenjis commented Aug 31, 2022

daycry commented Aug 31, 2022

daycry commented Aug 31, 2022

kenjis commented Sep 1, 2022

MGatner commented Sep 1, 2022

kenjis commented Sep 1, 2022

kenjis commented Sep 1, 2022

daycry commented Sep 1, 2022

daycry commented Sep 1, 2022

MGatner left a comment

MGatner Sep 2, 2022

MGatner Sep 2, 2022

MGatner Sep 2, 2022

MGatner Sep 2, 2022

MGatner Sep 2, 2022

MGatner Sep 2, 2022

daycry commented Sep 2, 2022 via email

MGatner left a comment

kenjis commented Sep 21, 2022

kenjis commented Sep 21, 2022

MGatner commented Sep 21, 2022

MGatner commented Sep 21, 2022

daycry commented Oct 11, 2022 via email

		@@ -2,10 +2,22 @@

		namespace CodeIgniter\Settings\Database\Migrations;

		use CodeIgniter\Config\BaseConfig;

	use CodeIgniter\Config\BaseConfig;
	use CodeIgniter\Settings\Config\Settings;

feat: Allow adding another database group #34

feat: Allow adding another database group #34

Conversation

daycry commented Aug 31, 2022

kenjis commented Aug 31, 2022

kenjis commented Aug 31, 2022

daycry commented Aug 31, 2022

kenjis commented Aug 31, 2022

daycry commented Aug 31, 2022

daycry commented Aug 31, 2022

kenjis commented Sep 1, 2022

MGatner commented Sep 1, 2022

kenjis commented Sep 1, 2022

kenjis commented Sep 1, 2022

daycry commented Sep 1, 2022

daycry commented Sep 1, 2022

MGatner left a comment

Choose a reason for hiding this comment

MGatner Sep 2, 2022

Choose a reason for hiding this comment

MGatner Sep 2, 2022

Choose a reason for hiding this comment

MGatner Sep 2, 2022

Choose a reason for hiding this comment

MGatner Sep 2, 2022

Choose a reason for hiding this comment

MGatner Sep 2, 2022

Choose a reason for hiding this comment

MGatner Sep 2, 2022

Choose a reason for hiding this comment

daycry commented Sep 2, 2022 via email

MGatner left a comment

Choose a reason for hiding this comment

kenjis commented Sep 21, 2022

kenjis commented Sep 21, 2022

MGatner commented Sep 21, 2022

MGatner commented Sep 21, 2022

daycry commented Oct 11, 2022 via email