Skip to content

ext/standard warning to error promotions - #5814

Closed
Girgias wants to merge 15 commits into
php:masterfrom
Girgias:standard-warning-to-error
Closed

ext/standard warning to error promotions#5814
Girgias wants to merge 15 commits into
php:masterfrom
Girgias:standard-warning-to-error

Conversation

@Girgias

Copy link
Copy Markdown
Member

Does should be rather uncontroversial I think

Comment threadext/standard/array.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

array_key_exists() is not a good target for this. Please see Nikita's comment here: #4887

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

isset does a usual int silent type cast, string array keys are the only weird thing where "03" will remain "03" but "3" would become the integer index 3 thus I think the behaviour would be consistent but can double check

Comment threadext/standard/dns.c Outdated
Comment threadext/standard/dns.c Outdated
Comment threadext/standard/streamsfuncs.c Outdated
@Girgias
Girgiasforce-pushed the standard-warning-to-error branch 2 times, most recently from 599506c to 2ead2f6CompareJuly 7, 2020 18:26
Comment threadext/standard/dns.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about cannot be longer than %d characters? As far as I see the condition, less than MAXFQDNLEN isn't true, because being equal is also okay.

Comment threadext/standard/dns.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
zend_argument_value_error(2, "must be between 1 and 65535 when Argument #5 ($raw) is true");
zend_argument_value_error(2, "must be between 1 and 65535 when argument #5 ($raw) is true");

@Girgias
Girgiasforce-pushed the standard-warning-to-error branch from 2ead2f6 to f9bd96aCompareAugust 1, 2020 23:09
@Girgias

Copy link
Copy Markdown
MemberAuthor

I dropped the array_key_exist one while addressing the reviews

Comment threadext/standard/dns.c Outdated
Comment threadext/standard/dns.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should list all possibilities here, as this may get extended and depends on environment (e.g. I don't think CAA works on Windows). Just say valid DNS record type or something like that.

Comment threadext/standard/dns.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd only mention DNS_* here (we do this in a few places).

Comment threadext/standard/iptc.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd go for is too large here, the exact value doesn't add value as it's impossible to reach with sensible code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These should be %d. Though I'm not really sure if we actually want to promote these. The number differs by platform and is something one could hit with reasonable code. As gethostbyname() is already fallible, it would currently be a necessary false check.

Comment threadext/standard/user_filters.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should just ZEND_ASSERT(fdat) here.

Comment threadext/standard/dns.c Outdated
Comment on lines 215 to 216

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

So compared to gethostbynamel(), gethostbyname() does not return false should I change this to return false if I revert the ValueError to warning?

@Girgias
Girgiasforce-pushed the standard-warning-to-error branch from e477a5d to c0aea24CompareSeptember 2, 2020 15:39
@Girgias
Girgias deleted the standard-warning-to-error branch September 3, 2020 15:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Girgias@nikic@kocsismate@carusogabriel