iframe-proxy

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

Allow adding another database group

- Allow adding another database group

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

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.

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

 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?

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


 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


 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


 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


 class AddContextColumn extends Migration
 {
+    private BaseConfig $config;


Suggested change

private BaseConfig $config;

private Settings $config;

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

- Improvements

8b0fe2c

- Allow adding another database group

kenjis reviewed Sep 1, 2022

View reviewed changes

Comment thread composer.json Outdated

kenjis reviewed Sep 1, 2022

View reviewed changes

Comment thread phpunit.xml.dist

kenjis reviewed Sep 1, 2022

View reviewed changes

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

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


		namespace CodeIgniter\Settings\Database\Migrations;

		use CodeIgniter\Config\BaseConfig;

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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

daycry commented Aug 31, 2022

Uh oh!

kenjis commented Aug 31, 2022

Uh oh!

kenjis commented Aug 31, 2022

Uh oh!

daycry commented Aug 31, 2022

Uh oh!

kenjis commented Aug 31, 2022

Uh oh!

daycry commented Aug 31, 2022

Uh oh!

daycry commented Aug 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kenjis commented Sep 1, 2022

Uh oh!

MGatner commented Sep 1, 2022

Uh oh!

kenjis commented Sep 1, 2022

Uh oh!

kenjis commented Sep 1, 2022

Uh oh!

daycry commented Sep 1, 2022

Uh oh!

daycry commented Sep 1, 2022

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

MGatner Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MGatner Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

MGatner Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

MGatner Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

MGatner Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

MGatner Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

daycry commented Sep 2, 2022 via email

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

kenjis commented Sep 21, 2022

Uh oh!

kenjis commented Sep 21, 2022

Uh oh!

MGatner commented Sep 21, 2022

Uh oh!

MGatner commented Sep 21, 2022

Uh oh!

daycry commented Oct 11, 2022 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants