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

Refactoring IpHelper #33

Open
wants to merge 24 commits into
base: 1.x-dev
Choose a base branch
from

Conversation

nibra
Copy link
Contributor

@nibra nibra commented Apr 1, 2021

Pull Request for Issue #32

Summary of Changes

  • Add tests verifying the bug. These tests reveal several other issues, see Drone report.

ATTN: This PR builds on PR #31

Testing Instructions

Run the tests (or just check Drone report).

Documentation Changes Required

No

@nibra nibra changed the title Refactoring/ip helper Refactoring IpHelper Apr 1, 2021
@nibra nibra marked this pull request as draft April 1, 2021 12:24
@nibra nibra linked an issue Apr 1, 2021 that may be closed by this pull request
@nibra nibra marked this pull request as ready for review April 1, 2021 21:52
@nibra nibra self-assigned this Apr 2, 2021
@nibra nibra requested a review from a team April 2, 2021 01:16
@Llewellynvdm
Copy link

I have been reading over the changes here and it all looks brilliant. Did not yet pull and test them manually.

I have one question you are making changes to a number of public functions very dramatically... what back-porting, or warning of change, do we have for those who have used those. As they are public, and therefore it does have a measure of promise, right?

@Llewellynvdm
Copy link

Llewellynvdm commented Apr 2, 2021

There are more... but I think you get my question. Not that I don't agree the changes are valid. Just indeed going to break stuff. Besides the fact that the whole class has mutated...

@nibra
Copy link
Contributor Author

nibra commented Apr 2, 2021

Those name changes do no harm, as method names are not case sensitive. The readability, though, is better with uppercase IP.

src/IpHelper.php Outdated Show resolved Hide resolved
@nibra
Copy link
Contributor Author

nibra commented Apr 2, 2021

The refactoring (and the added tests) changed the CRAP index from 8190 down to 53... Some parts of the original code were never able to run; it was trying to treat a netmask as an integer for IPv6 ranges...

@Llewellynvdm
Copy link

Yea, I get that... just trying to catch the guys like me who may have started using these methods directly...

@nibra
Copy link
Contributor Author

nibra commented Apr 2, 2021

API was restricted to the public methods; the class originally was final, so protected members were not accessible from outside.

src/IpHelper.php Outdated Show resolved Hide resolved
nibra added a commit that referenced this pull request Aug 14, 2022
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.

IpHelper does not detect invalid IP addresses
3 participants