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

Re-work the filters & Set the route name for routing in config #444

Merged
merged 23 commits into from
Jul 12, 2022

Conversation

mjamilasfihani
Copy link
Contributor

Fix issue #443

Copy link
Collaborator

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

See my comment on the issue, but this should be refactored with url_is() and route_to(), not relying on the App Config directly.

@mjamilasfihani
Copy link
Contributor Author

hang in there

@mjamilasfihani
Copy link
Contributor Author

I removed the helper('auth') and use loop to check the current url, in case if user has change their route name they just change the array. is it okay?

use `uri_string()` to get the string from current url
I use `current_url()` and at line 51 I add an extra script in case if user use index.php :)
@mjamilasfihani
Copy link
Contributor Author

mjamilasfihani commented Sep 26, 2021

if we use the CI version start from v4.0.5 the function url_is() is available, but if we end the support for < v4.0.5 I can just use url_is()

NOTE : Since url_is() is available start from CodeIgniter 4.0.5, user must update their application.
@mjamilasfihani
Copy link
Contributor Author

now we can use login filter in global :)

I remove the helper('auth) since it's no use in there (performance reason) and put the session check at the begin of code, because we need a session data right?
src/Filters/LoginFilter.php Outdated Show resolved Hide resolved
src/Filters/LoginFilter.php Outdated Show resolved Hide resolved
src/Filters/PermissionFilter.php Outdated Show resolved Hide resolved
src/Filters/PermissionFilter.php Outdated Show resolved Hide resolved
src/Filters/RoleFilter.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Collaborator

MGatner commented Sep 28, 2021

Looking better! A few changes. Also please be sure to run the style fixer.

@mjamilasfihani
Copy link
Contributor Author

Wait for a moment, I'll fix it

@mjamilasfihani mjamilasfihani changed the title Update LoginFilter.php Re-work the filters Sep 30, 2021
@mjamilasfihani
Copy link
Contributor Author

How about this PR @MGatner ? Any suggestion for the code position?

@MGatner
Copy link
Collaborator

MGatner commented Oct 3, 2021

It's too bulky for me to do on mobile, been waiting for a chance on desktop! From what I can see it looks right. I also want to check why tests are failing - I think it is unrelated to this PR.

@mjamilasfihani
Copy link
Contributor Author

Okay, I hope it'll fix soon

@mjamilasfihani
Copy link
Contributor Author

Hello @MGatner how the PHPStan's status? Still not working?

@MGatner
Copy link
Collaborator

MGatner commented Oct 30, 2021

The PHPStan issues aren't related to this PR - probably an update to the tool itself caused this. I'm good with this change! @lonnieezell last look?

@mjamilasfihani
Copy link
Contributor Author

@MGatner I am happy to hear that :)

Copy link
Collaborator

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Okay I made a proper review this time! A few suggestions to clean up and make this a legit refactor 😉

src/Filters/LoginFilter.php Outdated Show resolved Hide resolved
src/Filters/LoginFilter.php Outdated Show resolved Hide resolved
src/Filters/RoleFilter.php Outdated Show resolved Hide resolved
@mjamilasfihani
Copy link
Contributor Author

Okay I made a proper review this time! A few suggestions to clean up and make this a legit refactor 😉

Thanks for the review, I will try to figure it :)

it's funny, I just learned about abstract class and you give that suggestion 😂

@lonnieezell
Copy link
Owner

I have no problems with this PR, I think. I agree with @MGatner latest comments so go ahead and address those, though.

@mjamilasfihani mjamilasfihani changed the title Re-work the filters Re-work the filters & Pre-Config for Routing Nov 3, 2021
@mjamilasfihani
Copy link
Contributor Author

fix #453

@mjamilasfihani mjamilasfihani changed the title Re-work the filters & Pre-Config for Routing Re-work the filters & Set the route name for routing in config Nov 3, 2021
I add defaultLandingRoute to store the default route name after user success to login (depend on the real case) or fail in Permission & Role Filters too
src/Config/Auth.php Outdated Show resolved Hide resolved
@mjamilasfihani
Copy link
Contributor Author

Any idea how I fail all the test :( @MGatner

@MGatner
Copy link
Collaborator

MGatner commented Nov 19, 2021

@mjamilasfihani This repos tests might be borked. I would like to switch the CI/CD to the new CodeIgniter Dev Kit which hopefully will be ready today or tomorrow.

@mjamilasfihani
Copy link
Contributor Author

That's good. Anytime you can do it @MGatner

@MGatner
Copy link
Collaborator

MGatner commented Apr 27, 2022

Hi @mjamilasfihani - a 5-month hiatus, but I am almost ready to merge the DevKit updates. Are you around to rebase this and clean up any changes for a final review once #517 is in?

@MGatner MGatner closed this Apr 27, 2022
@MGatner MGatner reopened this Apr 27, 2022
@mjamilasfihani
Copy link
Contributor Author

Thanks @MGatner for the good news, I'm ready for this 🔥

The Routes.php now will use reserved routing from Auth.php
Copy link
Collaborator

@MGatner MGatner 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 good! Please add some tests. The framework now has the FilterTestTrait which makes it easy to test filters.

src/Config/Auth.php Outdated Show resolved Hide resolved
@MGatner MGatner merged commit 100822e into lonnieezell:develop Jul 12, 2022
@MGatner MGatner mentioned this pull request Jul 13, 2022
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.

None yet

4 participants