Skip to content

Conversation

@MGatner
Copy link
Member

@MGatner MGatner commented Nov 30, 2020

Description

This is an attempt at a gentler approach to moving our HTTP layer towards PSR compliance. This includes a number of deprecations, which I will note in the User Guide and CHANGELOG once we've had some time to discuss this.

See https://github.com/codeigniter4/CodeIgniter4/projects/5

  • Splits out non-PSR-7 HTTP class methods into traits
  • Deprecates unnecessary or conflicting methods
  • Reimplements framework to remove deprecated methods (partial)
  • Adds MessageInterface to clarify child class requirements

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@lonnieezell
Copy link
Member

A little hard to see sometimes with so many changes to those files, but in general I'm ok with this solution, I think. Nice job.

@MGatner
Copy link
Member Author

MGatner commented Dec 1, 2020

A little hard to see sometimes with so many changes to those files

I'm trying to split things into logical commit chunks to make the review easier. I could see some merit in splitting this into a few different PRs: deprecations, trait splits, Response refactor. Let me know if you'd prefer that.

@lonnieezell
Copy link
Member

I could see some merit in splitting this into a few different PRs: deprecations, trait splits, Response refactor. Let me know if you'd prefer that.

Nah. You're good as is.

@MGatner
Copy link
Member Author

MGatner commented Dec 1, 2020

GitHub Actions appears to be bugging out. I think the tests are all passing but someone should rerun at a later time.

@MGatner MGatner closed this Dec 2, 2020
@MGatner MGatner reopened this Dec 2, 2020
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I trust all is well for this big PSR-7 compliance.

Some notes below.

@samsonasik
Copy link
Member

Documentation PSR compliance may need to be updated http://codeigniter.com/user_guide/intro/psr.html

@MGatner
Copy link
Member Author

MGatner commented Dec 7, 2020

I trust all is well for this big PSR-7 compliance.

Documentation PSR compliance may need to be updated http://codeigniter.com/user_guide/intro/psr.html

Just to be clear, this is by no means PSR-7 compliant yet. The deprecations and interface updates will make space for a new layer of PSR-7-compliant classes but current conflicts prevent us from implementing those. Also, I don't see the framework itself using a compliant HTTP layer because there is currently too much built into the Message/Request/Response classes that breaks compliance. The short-term plan is to apply those requirements onto any PSR-7 set of classes to make them work with the framework, and so third-party packages could use the framework's intermediate layer instead of adding another one.

MGatner and others added 2 commits December 7, 2020 12:03
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
@MGatner MGatner requested a review from paulbalandan December 7, 2020 17:09
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Looks okay to me, just with very minor fixes.

@MGatner MGatner requested a review from paulbalandan December 21, 2020 20:26
@MGatner MGatner merged commit 62ff134 into codeigniter4:develop Dec 21, 2020
@MGatner MGatner deleted the http-agnostic branch December 21, 2020 20:51
@MGatner MGatner restored the http-agnostic branch December 21, 2020 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants