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

Ip validator and Ip tests renovation #32

Merged
merged 6 commits into from Nov 13, 2019
Merged

Conversation

kamarton
Copy link
Contributor

@kamarton kamarton commented Nov 1, 2019

Q A
Is bugfix? ✔️
New feature? ✔️
Breaks BC? ✔️
Tests pass? ✔️
Fixed issues -
Alternative PR #21 (abandoned)

@kamarton kamarton changed the title Ip validator and Ip tests renovation [WIP] Ip validator and Ip tests renovation Nov 1, 2019
@kamarton
Copy link
Contributor Author

@kamarton kamarton commented Nov 1, 2019

@samdark questions

  • Does it make sense to allow leading zeros for IPv4? (10.001.000002.1) https://superuser.com/a/929180/1082613
    In my opinion, it makes no sense and only raises additional problems, eg. if allowed, then abnormal IP addresses will also pass through. 01.01.01.01, 001,001,001,001, 0000000000001.0000000000001.000000000001.00000000001.
    UPD:
$ php -r 'var_dump(inet_pton("01.01.01.01"));'
PHP Warning:  inet_pton(): Unrecognized address 01.01.01.01 in Command line code on line 1
PHP Stack trace:
PHP   1. {main}() Command line code:0
PHP   2. inet_pton() Command line code:1
Command line code:1:
bool(false)

$this->assertTrue($validator->validate('192.168.005.001'));

  • In the original code, the subnet had three states: optional, require, not allow. Since cannot modify the input value, required state does not make sense. 127.0.0.1 === 127.0.0.1 / 32.
    For example firewall in GCP:
    firewall example

@kamarton
Copy link
Contributor Author

@kamarton kamarton commented Nov 1, 2019

Tests with FakedValidationModel style is relevant?

@samdark
Copy link
Member

@samdark samdark commented Nov 2, 2019

Does it make sense to allow leading zeros for IPv4?

No.

In the original code, the subnet had three states: optional, require, not allow. Since cannot modify the input value, required state does not make sense. 127.0.0.1 === 127.0.0.1 / 32.

OK.

Tests with FakedValidationModel style is relevant?

No.

@kamarton kamarton changed the title [WIP] Ip validator and Ip tests renovation Ip validator and Ip tests renovation Nov 6, 2019
@kamarton
Copy link
Contributor Author

@kamarton kamarton commented Nov 6, 2019

Finally, I left all the subnet options (not allow, reuqired, optional). Reviewable.

Desktop screenshot

@kamarton
Copy link
Contributor Author

@kamarton kamarton commented Nov 6, 2019

The Ip::getRanges() function may not be required. It is currently used only for tests, but important for them.

@samdark samdark added the status:code review label Nov 11, 2019
@samdark
Copy link
Member

@samdark samdark commented Nov 12, 2019

Any idea why travis builds fail?

composer.json Outdated
"ext-intl": "*"
},
"suggest": {
Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

If the package is required, there's no sense in suggesting it.

Copy link
Contributor Author

@kamarton kamarton Nov 13, 2019

Choose a reason for hiding this comment

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

removed

*/
public function setRanges($ranges)
public function ranges(array $ranges)
Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

Suggested change
public function ranges(array $ranges)
public function ranges(array $ranges): self

Copy link
Contributor Author

@kamarton kamarton Nov 13, 2019

Choose a reason for hiding this comment

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

Phpdoc @return static is a better solution because self must be compliance with the parent class name after inheritance.

example

MyExtendendIp extends Ip {         ˇˇ----- the PHP processor throws an error if I do not use the parent class.
  public function ranges($ranges): Ip {
     // overerride
  }
}

Of course, this is not a problem if we set the Ip class to final.

src/Rule/Ip.php Outdated
/**
* @return static
*/
public function allowIpv4(bool $value = true)
Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

Suggested change
public function allowIpv4(bool $value = true)
public function allowIpv4(bool $value = true): self

src/Rule/Ip.php Outdated
/**
* @return static
*/
public function allowIpv6(bool $value = true)
Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

Suggested change
public function allowIpv6(bool $value = true)
public function allowIpv6(bool $value = true): self

src/Rule/Ip.php Outdated
/**
* @return static
*/
public function allowNegation(bool $value = true)
Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

Suggested change
public function allowNegation(bool $value = true)
public function allowNegation(bool $value = true): self

Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

How about allowNegation and disallowNegation?

Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

Same w/ other allow* methods.

Copy link
Contributor Author

@kamarton kamarton Nov 13, 2019

Choose a reason for hiding this comment

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

How about allowNegation and disallowNegation?

like it.

Copy link
Contributor Author

@kamarton kamarton Nov 13, 2019

Choose a reason for hiding this comment

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

added

src/Rule/Ip.php Outdated
*/
private function validateSubnet(string $ip)
public function allowSubnet(bool $allow = true, bool $required = false)
Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

Suggested change
public function allowSubnet(bool $allow = true, bool $required = false)
public function allowSubnet(bool $allow = true, bool $required = false): self

Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

Also it seems to try setting two separate things at the same time. How about allowSubnet(), requireSubnet(), disallowSubnet()?

Copy link
Contributor Author

@kamarton kamarton Nov 13, 2019

Choose a reason for hiding this comment

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

How about allowSubnet(), requireSubnet(), disallowSubnet()?

Like it. It is much clearer to use.

Copy link
Contributor Author

@kamarton kamarton Nov 13, 2019

Choose a reason for hiding this comment

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

added

@kamarton
Copy link
Contributor Author

@kamarton kamarton commented Nov 13, 2019

Any idea why travis builds fail?

The other classes in the package are not ready yet, so the tests will fail.

@kamarton
Copy link
Contributor Author

@kamarton kamarton commented Nov 13, 2019

depend on yiisoft/network-utilities#13

@samdark samdark merged commit 3e67611 into yiisoft:master Nov 13, 2019
0 of 3 checks passed
@samdark
Copy link
Member

@samdark samdark commented Nov 13, 2019

Merged. Thank you!

@kamarton kamarton mentioned this pull request Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants