Skip to content

Fixed filters exec order#7404

Closed
c4user wants to merge 1 commit into
codeigniter4:developfrom
c4user:develop
Closed

Fixed filters exec order#7404
c4user wants to merge 1 commit into
codeigniter4:developfrom
c4user:develop

Conversation

@c4user

@c4user c4user commented Apr 3, 2023

Copy link
Copy Markdown

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

kenjis commented Apr 4, 2023

Copy link
Copy Markdown
Member

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

MGatner commented Jul 3, 2023

Copy link
Copy Markdown
Member

@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

kenjis commented Sep 21, 2023

Copy link
Copy Markdown
Member

I sent a PR based on this. See #7955

@kenjis

kenjis commented Oct 11, 2023

Copy link
Copy Markdown
Member

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