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

Deprecate IPUtils in favour of symfony/http-foundation #10846

Closed
2 of 3 tasks
GuySartorelli opened this issue Jun 28, 2023 · 3 comments
Closed
2 of 3 tasks

Deprecate IPUtils in favour of symfony/http-foundation #10846

GuySartorelli opened this issue Jun 28, 2023 · 3 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Jun 28, 2023

The IPUtils class is a direct copy from the class of the same name in symfony/http-foundation.
We should deprecate our class, and just include symfony/http-foundation as a direct dependency, directing people to use it instead.

Reasoning

  1. Currently some of our methods in that class are out of date, and don't have improvements Symfony has made. Using their package directly obviously keeps it up to date.
  2. We're left maintaining code that someone else is already maintaining. That's just silly.
  3. In general, there's a desire currently to offload more of our code to third-party libraries which are more mature and better situated to maintain that code. This aligns with that desire.

Acceptance criteria

  • Remove the old code and replace it with symfony dependency
  • Deprecate any code as necessary in CMS 5
  • If we can simply swap it out and no unit tests break, then do the work directly in CMS 5 instead. Otherwise if we need to update the code that calls these methods, then target CMS 6

CMS 5 PRs

CMS 6 PRs

@GuySartorelli
Copy link
Member Author

@emteknetnz Please create a PR for the 5.3 changelog that says IPUtils has been deprecated, and to use the symfony class instead.

@GuySartorelli
Copy link
Member Author

CMS 5 PRs merged. Reassigning to Steve for CMS 6 PRs

@GuySartorelli
Copy link
Member Author

PRs merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants