Skip to content

Notice and Warning promotion in ext/zip - #5823

Closed
Girgias wants to merge 8 commits into
php:masterfrom
Girgias:zip-error-promotion
Closed

Notice and Warning promotion in ext/zip#5823
Girgias wants to merge 8 commits into
php:masterfrom
Girgias:zip-error-promotion

Conversation

@Girgias

Copy link
Copy Markdown
Member

IMHO Most of these notices should have been warnings already as similar error states to those were warnings in other extensions, therefore I also promoted them to ValueErrors

@remicollet

Copy link
Copy Markdown
Member

Sorry but I don't like this one, especially the php_zip_parse_options part.

In previous versions, we don't have any notice/warning

And I don't think we should be stricter than function arg parsing, so only raise exception in strict mode.

@Girgias

Copy link
Copy Markdown
MemberAuthor

Sorry but I don't like this one, especially the php_zip_parse_options part.

In previous versions, we don't have any notice/warning

Are these notices/warnings new in PHP 8?

And I don't think we should be stricter than function arg parsing, so only raise exception in strict mode.

I'm not sure what you mean by this? PHP 8 will throw type errors even for internal functions and not return null + warning.

@remicollet

Copy link
Copy Markdown
Member

I mean that

<?php
function run(bool $b) {
echo "$b" ? "Yes\n" : "No\n";
}
run(1);

run without any error.

Comment threadext/zip/php_zip.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.

it is where I say we have no waning in previous version.

@Girgias

Copy link
Copy Markdown
MemberAuthor

Okay, I see now, so if I drop the changes in php_zip_parse_options() would it be more acceptable?

@remicollet

remicollet commented Jul 10, 2020

Copy link
Copy Markdown
Member

I'm mostly fine with TypeError, but only in strict mode.

I will read other change after the week-end.

@Girgias
Girgiasforce-pushed the zip-error-promotion branch from 6132ee3 to be98aaeCompareJuly 20, 2020 10:36
Comment threadext/zip/php_zip.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.

Is there any precedent for this? I don't remember us making strict_types distinctions for something other than parameters anywhere.

@nikic

nikic commented Aug 6, 2020

Copy link
Copy Markdown
Member

Apart from php_zip_parse_options() this looks pretty good to me. For php_zip_parse_options() I would suggest not to mess with strict_types (which I think we don't do anywhere else) and only add the warnings. The warnings can be converted to errors at a later time then.

@nikicnikic left a comment

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.

This looks good to me, but maybe @remicollet wants to take another look as well.

Comment threadext/zip/php_zip.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.

Sorry, but I don't see any benefit changing this, and it will make merge from lower version harded and risky.

Comment threadext/zip/php_zip.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.

please remove all these TODO.
unrelate to this PR
And returning FALSE is the documented behavior for not found entry.

@Girgias
Girgiasforce-pushed the zip-error-promotion branch from bb12ed7 to d187b73CompareAugust 17, 2020 12:47
@Girgias
Girgiasforce-pushed the zip-error-promotion branch from d187b73 to 3364f8bCompareAugust 17, 2020 12:52
@remicollet

remicollet commented Aug 17, 2020

Copy link
Copy Markdown
Member

Thanks,

Squashed and merged

72383cc

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@remicollet@nikic@carusogabriel