Skip to content

Promote some warnings in MBString Regex - #5341

Closed
Girgias wants to merge 7 commits into
php:masterfrom
Girgias:mbstring-regex-warnings-to-error
Closed

Promote some warnings in MBString Regex#5341
Girgias wants to merge 7 commits into
php:masterfrom
Girgias:mbstring-regex-warnings-to-error

Conversation

@Girgias

@GirgiasGirgias commented Apr 2, 2020

Copy link
Copy Markdown
Member

I'm clueless on some of them if they should be converted or not.

nikic
nikic reviewed Apr 3, 2020
View reviewed changes
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
@Girgias
Girgiasforce-pushed the mbstring-regex-warnings-to-error branch from c9a14a3 to 2ad2671CompareApril 3, 2020 15:49
Girgias
Girgias commented Apr 3, 2020
View reviewed changes
Comment threadext/mbstring/tests/bug72399.phpt Outdated
Comment on lines 15 to 24

@GirgiasGirgiasApr 3, 2020

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.

I really don't get why this throws, will need to investigate I suppose.

@nikicnikicSep 8, 2020

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.

Why shouldn't it throw?

@GirgiasGirgiasSep 8, 2020

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.

I don't know was confused, probably because I suppose it should throw before? As in in mb_ereg_search_init() ?

@Girgias
Girgiasforce-pushed the mbstring-regex-warnings-to-error branch 2 times, most recently from 7903ef5 to 54b6ac2CompareApril 9, 2020 22:18
@Girgias
Girgias requested a review from nikicApril 9, 2020 22:21
@Girgias
Girgiasforce-pushed the mbstring-regex-warnings-to-error branch from 54b6ac2 to 8092994CompareApril 10, 2020 16:13
@Girgias
Girgiasforce-pushed the mbstring-regex-warnings-to-error branch from 8092994 to 2aa6653CompareApril 23, 2020 21:47
kocsismate
kocsismate reviewed Aug 1, 2020
View reviewed changes
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
@nikic

nikic commented Aug 6, 2020

Copy link
Copy Markdown
Member

I think another PR also did something in this area, can you please rebase?

nikic
nikic reviewed Aug 6, 2020
View reviewed changes
Comment threadext/mbstring/php_mbregex.c Outdated
nikic
nikic reviewed Aug 6, 2020
View reviewed changes
Comment threadext/mbstring/php_mbregex.c Outdated
@Girgias
Girgiasforce-pushed the mbstring-regex-warnings-to-error branch from 2aa6653 to 5d36f8aCompareAugust 6, 2020 14:07
@GirgiasGirgias mentioned this pull request Aug 13, 2020
Closed
71 tasks
nikic
nikic reviewed Sep 8, 2020
View reviewed changes
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
@Girgias
Girgiasforce-pushed the mbstring-regex-warnings-to-error branch from 5d36f8a to fcd52e5CompareSeptember 8, 2020 13:46
nikic
nikic reviewed Sep 8, 2020
View reviewed changes
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/php_mbregex.c Outdated
Comment threadext/mbstring/tests/bug72399.phpt Outdated
Comment on lines 15 to 24

@nikicnikicSep 8, 2020

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.

Why shouldn't it throw?

@Girgias
Girgiasforce-pushed the mbstring-regex-warnings-to-error branch from fcd52e5 to fdac9bdCompareSeptember 8, 2020 15:36
nikic
nikic approved these changes Sep 9, 2020
View reviewed changes
Comment threadext/mbstring/php_mbregex.c
case 'e':
if (eval != NULL) *eval = 1;
break;
zend_value_error("Option \"e\" is not supported");

@nikicnikicSep 9, 2020

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.

You can remove this as a separate case (handle in default). Or alternatively, use a different error message like "no longer supported".

@Girgias
Girgias deleted the mbstring-regex-warnings-to-error branch September 9, 2020 12:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kocsismatekocsismatekocsismate left review comments
+1 more reviewer
@nikicnikicnikic approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Warning promotion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Girgias@nikic@kocsismate@carusogabriel