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.

Sign up for GitHub

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

Jump to bottom

feat: new Required Filters #8053

Merged
merged 32 commits into from
Nov 19, 2023
Merged

feat: new Required Filters #8053

merged 32 commits into from
Nov 19, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Oct 17, 2023 •
edited
Loading

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 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
Copy link
Member

@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.

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
Copy link

👋 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 removed the stale Pull requests with conflicts label Oct 22, 2023
@kenjis
Copy link
Member Author

kenjis commented Oct 30, 2023

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

@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Nov 3, 2023
@kenjis
Copy link
Member Author

kenjis commented Nov 3, 2023

Moved forceSecureAccess() logic to ForceHTTPS filter.

MGatner
MGatner requested changes Nov 4, 2023
View reviewed changes
Copy link
Member

@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.

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

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

MGatner commented Nov 5, 2023

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

@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
Copy link
Member Author

kenjis commented Nov 19, 2023

The Psalm error is not related to this PR.

@kenjis kenjis merged commit b5451cb into codeigniter4:4.5 Nov 19, 2023
45 of 46 checks passed
@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
4.5 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