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

WIP migration to CI4 #1273

Merged
merged 288 commits into from
Nov 28, 2018
Merged

WIP migration to CI4 #1273

merged 288 commits into from
Nov 28, 2018

Conversation

bvrignaud
Copy link
Contributor

@bvrignaud bvrignaud commented Oct 1, 2018

This is a migration to CI4 (#1270).

It's works but there are still things to do.

Maybe rename functions an d variables in camelCase to be compatible with CI 4 coding standard.

TODO :
[] make unit test
[] make installation with composer

Copy link
Owner

@benedmunds benedmunds left a comment

Choose a reason for hiding this comment

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

Left a couple comments, I know you're in progress so feel free to ignore if you're already aware.

Agreed that camelCasing and tests are a good idea.

/**
* @return array A CSRF key-value pair
*/
public function _get_csrf_nonce(): array
Copy link
Owner

Choose a reason for hiding this comment

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

I think this can be refactored away now that CI offerers CSRF built in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You 'are right.
We can remove CSRF in the branch '3' to.

| For more information, check the password_hash function help: http://php.net/manual/en/function.password-hash.php
|
*/
public $hash_method = 'bcrypt'; // bcrypt or argon2
Copy link
Owner

Choose a reason for hiding this comment

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

Need to this the spacing here down to like 126

@benedmunds
Copy link
Owner

Looking really good. Lmk when you're ready and I'll set aside some time to do some manual testing and poking around.

@bvrignaud
Copy link
Contributor Author

Hi,

You can look the advancement : https://github.com/bvrignaud/CodeIgniter-Ion-Auth/tree/4
It's still WIP.

Copy link
Owner

@benedmunds benedmunds left a comment

Choose a reason for hiding this comment

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

This is looking great. Left a couple comments. Looking through this, it wouldn't be difficult to make this a generic PHP project from this point.

If you're up for that, I think the main effort is injecting all the dependencies. Those dependencies can still be CI4 libraries. If you arent up for that, no worries, this will work for CI4 and we could cut a new version thats generic.

Config/IonAuth.php Outdated Show resolved Hide resolved
INSTALLING.md Outdated Show resolved Hide resolved
INSTALLING.md Outdated Show resolved Hide resolved
INSTALLING.md Show resolved Hide resolved
@bvrignaud
Copy link
Contributor Author

whats the reason for changing the namespacing? I like the idea of everything living under the IonAuth namespace, but this might be required for the config?

I like it to, but I think there is still a bug in CodeIgniter 4 develop with loading config file. I Had created a new issue : codeigniter4/CodeIgniter4#1293

I like the idea to have the generic config file in module/config directory and if we want to personalize it, we can override it in the application/config directory.

@benedmunds
Copy link
Owner

Merging whats here so far

@benedmunds benedmunds merged commit fd1fd75 into benedmunds:4 Nov 28, 2018
@benedmunds
Copy link
Owner

@bvrignaud what is the status here? need help with unit testing or anything else?

@bvrignaud
Copy link
Contributor Author

Good news.

It's usable, I use it on a new project. But I think there are still many things to do.
I'm using only a small parts of the library.
I wrote some unit tests, but I think could be nice to use seeds an CI workflow.

Go on guys, try it and improve it !

@benedmunds
Copy link
Owner

Awesome, I'll get the word out.

@tpw1314
Copy link
Contributor

tpw1314 commented Nov 29, 2018

Excellent. Will try it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants