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

Fixed filters exec order #7404

Closed
wants to merge 1 commit into from
Closed

Conversation

c4user
Copy link

@c4user c4user commented Apr 3, 2023

Exec order of the filters was rather irregular. This can cause performance problems. As defaults in config, the csrf filter is always run after the router filters. Let's say you are using a database connection in router filters and the data sent is not suitable for csrf but the filters are working. or you need to use errorhandler for php8 (flood warning), you can't use it in global filter. Because you cannot catch errors in router filters.

before filters:
Previously: routers > global > method > filters
Now: global > method > filters > routers

after filters:
Previously: routers > global > filters
Now: routers > filters > global

@c4user c4user changed the title Filters's is execute order fixed. Fixed filters exec order Apr 3, 2023
@kenjis kenjis added wrong branch PRs sent to wrong branch breaking change Pull requests that may break existing functionalities tests needed Pull requests that need tests GPG-Signing needed Pull requests that need GPG-Signing labels Apr 4, 2023
@kenjis
Copy link
Member

kenjis commented Apr 4, 2023

Thank you for trying to improve CI4.

But this PR has missing items.

  1. GPG siginig
  2. Test code

Please read https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md

And we don't accept breaking changes in develop as much as possible.
At least this PR should go to 4.4 branch.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#if-you-sent-to-the-wrong-branch

Also please read the past discussion #6262

@MGatner
Copy link
Member

MGatner commented Jul 3, 2023

@c4user are you available to address kenjis' concerns above?

@kenjis kenjis added the stale Pull requests with conflicts label Aug 27, 2023
@kenjis kenjis mentioned this pull request Sep 20, 2023
5 tasks
@kenjis
Copy link
Member

kenjis commented Sep 21, 2023

I sent a PR based on this. See #7955

@kenjis
Copy link
Member

kenjis commented Oct 11, 2023

Closed by #7955

@kenjis kenjis closed this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities GPG-Signing needed Pull requests that need GPG-Signing stale Pull requests with conflicts tests needed Pull requests that need tests wrong branch PRs sent to wrong branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants