Skip to content

Conversation

sammyskills
Copy link
Contributor

Fixes #798

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

I am on mobile. I did not check the issue practically.

But the explanation and how to implement it seems logical.

@sammyskills
Copy link
Contributor Author

Thanks @datamweb. Feel free to check when you can.

@sammyskills
Copy link
Contributor Author

The option for adding arguments to filter configuration has been added to CI v4.4.0, see docs.

This PR might need to be updated to either:

  1. Update the docs and encourage developers to upgrade to v4.4.0
  2. Maintain this PR to cover all use cases.

WDYT?

@datamweb
Copy link
Collaborator

The option for adding arguments to filter configuration has been added to CI v4.4.0, see docs.

Yes, I remembered, I had reviewed this one😀.

  1. Update the docs and encourage developers to upgrade to v4.4.0

Shield requires CodeIgniter 4.2.7+ at this point I am not very inclined to upgrade to 4.4.0.
The difference between CodeIgniter 4.2.7+ and 4.4.0 is huge, 4.4.0 is great, but I think we shouldn't limit the user community at this point.

@sammyskills
Copy link
Contributor Author

Hi @kenjis, @lonnieezell, @MGatner can you take a look?

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 kenjis added the bug Something isn't working label Sep 5, 2023
@sammyskills sammyskills requested a review from datamweb September 5, 2023 20:25
@datamweb
Copy link
Collaborator

datamweb commented Sep 5, 2023

@sammyskills is aware of this, I haven't played with the code yet, I'll try to do it tomorrow.

Comment on lines +35 to +36
$session = session();
$session->setTempdata('beforeLoginUrl', current_url(), 300);
Copy link
Member

Choose a reason for hiding this comment

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

I just came up with this code, but it is a copy and paste.
Since it is the same knowledge (save the URL for 300 seconds in the session), it might be better to put it together in a method if possible.

But there doesn't seem to be an appropriate class to put it in now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I'm currently merge this. Maybe we can refactor later.

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

@sammyskills I came back after a million days 😃. Sorry.
Thank you!

@datamweb datamweb merged commit 319b5ef into codeigniter4:develop Sep 7, 2023
@sammyskills sammyskills deleted the fix-unauthorized-users-before-login-url branch September 7, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: beforeLoginUrl is not stored in session if a user is logged out while in a protected group route.
3 participants