Skip to content

feat: new Required Filters#8053

Merged
kenjis merged 32 commits into
codeigniter4:4.5from
kenjis:feat-required-filters
Nov 19, 2023
Merged

feat: new Required Filters#8053
kenjis merged 32 commits into
codeigniter4:4.5from
kenjis:feat-required-filters

Conversation

@kenjis

@kenjis kenjis commented Oct 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Needs #8050

Description

  • add "Required Filters" that always run (before filters) before Page Cache check and routing
  • add ForceHTTPS, PageCache and PerformanceMetrics as "Required Filters"
  • fix benchmark points

Changes

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.5 labels Oct 17, 2023
@kenjis kenjis marked this pull request as draft October 17, 2023 07:45
@kenjis kenjis force-pushed the feat-required-filters branch from abf34fa to 6c4d18e Compare October 17, 2023 08:18
@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Oct 17, 2023
MGatner
MGatner reviewed Oct 17, 2023
View reviewed changes

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll have to come back to this one; I like the concept.

@kenjis kenjis force-pushed the feat-required-filters branch 6 times, most recently from fb76b76 to 9d23781 Compare October 20, 2023 20:16
@github-actions github-actions Bot added the stale Pull requests with conflicts label Oct 22, 2023
@github-actions

github-actions Bot commented Oct 22, 2023

Copy link
Copy Markdown

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis force-pushed the feat-required-filters branch from 9d23781 to 5ad536a Compare October 22, 2023 22:04
@kenjis kenjis removed the stale Pull requests with conflicts label Oct 22, 2023
@kenjis kenjis force-pushed the feat-required-filters branch from 5ad536a to df3d93a Compare October 22, 2023 22:26
@kenjis

kenjis commented Oct 30, 2023

Copy link
Copy Markdown
Member Author

The implementation is completed. If this may go, I will write docs.

@kenjis kenjis force-pushed the feat-required-filters branch from df3d93a to 0d52c90 Compare November 3, 2023 02:46
@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Nov 3, 2023
@kenjis

kenjis commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

Moved forceSecureAccess() logic to ForceHTTPS filter.

MGatner
MGatner requested changes Nov 4, 2023
View reviewed changes

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't looked at tests yet but so far this is looking good. A few comments.

Comment thread app/Config/Filters.php Outdated
Comment thread system/CodeIgniter.php
Comment thread system/Filters/Filters.php Outdated
Comment thread system/Filters/Filters.php Outdated
@kenjis kenjis force-pushed the feat-required-filters branch 5 times, most recently from debc476 to 19271ee Compare November 5, 2023 02:15
@MGatner

MGatner commented Nov 5, 2023

Copy link
Copy Markdown
Member

Thanks for the changes. I will do a final review when draft is done.

@kenjis kenjis force-pushed the feat-required-filters branch from 7cf7da3 to e871e65 Compare November 9, 2023 05:02
@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Nov 9, 2023
kenjis and others added 23 commits November 20, 2023 06:06
@kenjis
refactor: fix PHPStan errors
005b95f
@kenjis
refactor: remove meaningless code
a4dd669
@kenjis
fix: forceSecureAccess() position
c0d7751 
It was before the event `pre_system`.
@kenjis
docs: add @return
df9a5f8
@kenjis @MGatner
docs: fix by proofreading
2a4e587 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis @MGatner
refactor: use single iteration
db582a2 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis
refactor: move forceSecureAccess() to ForceHTTPS filter
a4998c2
@kenjis
docs: add comments to Config\Filters
2c497d9
@kenjis
docs: add about Required Filters
10b6cad
@kenjis
docs: update Filter Execution Order
91994fa
@kenjis
docs: add about ForceHTTPS and PerformanceMetrics
7646a44
@kenjis
docs: change TOC depth to 3
0cc0654
@kenjis
docs: update filter property name
57fb443
@kenjis
docs: add changelog
af366ad
@kenjis
docs: add upgrade
cb2516a
@kenjis @MGatner
docs: fix by proofreading
8aa9169 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis
docs: add @internal
523e414
@kenjis
config: add system/Config/Filters.php
71cc57a
@kenjis
refactor: extract setToolbarToLast()
baeea0f
@kenjis
feat: to work even if Config\Filters is not updated
64d6965
@kenjis
fix: toolbar does not work in globals filters
1217561
@kenjis
docs: app/Config/Filters.php changes are not mandatory
3ed4329
@kenjis
docs: add @phpstan-ignore-line
709b9f2
@kenjis kenjis force-pushed the feat-required-filters branch from cbce137 to 709b9f2 Compare November 19, 2023 21:12
@kenjis

kenjis commented Nov 19, 2023

Copy link
Copy Markdown
Member Author

The Psalm error is not related to this PR.

@kenjis kenjis merged commit b5451cb into codeigniter4:4.5 Nov 19, 2023
@kenjis kenjis deleted the feat-required-filters branch November 19, 2023 22:08
This was referenced Dec 15, 2023
Merged
Merged
@kenjis kenjis mentioned this pull request Feb 7, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MGatner MGatner MGatner approved these changes

Assignees

No one assigned

Labels

breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kenjis @MGatner
⚡