Skip to content

Mysqli warning promotion - #5803

Closed
Girgias wants to merge 12 commits into
php:masterfrom
Girgias:mysqli-warning-promotion
Closed

Mysqli warning promotion#5803
Girgias wants to merge 12 commits into
php:masterfrom
Girgias:mysqli-warning-promotion

Conversation

@Girgias

Copy link
Copy Markdown
Member

I will split this extension into multiple PRs as it's already starting to become rather large.

Comment threadext/mysqli/mysqli_api.c Outdated
Comment threadext/mysqli/mysqli_api.c Outdated
@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch 9 times, most recently from d1d1ac2 to 6131745CompareJuly 9, 2020 18:04
@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch 2 times, most recently from f5e67f1 to 777f11cCompareJuly 20, 2020 12:49
@GirgiasGirgias changed the title Mysqli warning promotion part 1Mysqli warning promotionJul 20, 2020
Comment threadext/mysqli/mysqli_nonapi.c Outdated
@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch from 777f11c to 99ac5b7CompareAugust 3, 2020 01:48
@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch 3 times, most recently from d86e165 to 67e6b6aCompareAugust 23, 2020 22:10
@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch 4 times, most recently from 4d53fdc to 1e66bebCompareSeptember 9, 2020 22:36
@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch 3 times, most recently from 26cdc04 to 15f2f4bCompareSeptember 13, 2020 01:15
Comment threadext/mysqli/mysqli.c Outdated
Comment threadext/mysqli/mysqli_api.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 think a few the is missing from this message. :) I would say The number of elements in the type definition string doesn't match the number of bind variables, but maybe it's too much now (?). Otherwise, you could say argument #2 (...) instead of type definition string.

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.

Besides, you could use the ... must match the number of bind variables format, which is the most common wording we use (rather than ... doesn't match ...).

Comment threadext/mysqli/mysqli_api.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.

Yes, I do think it makes sense to forbid this case. The other PR handles this though.

Comment threadext/mysqli/mysqli_api.c Outdated
Comment threadext/mysqli/mysqli_api.c Outdated
@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch 2 times, most recently from 4b52075 to 717d1fdCompareSeptember 14, 2020 00:58
Comment threadext/mysqli/mysqli.c Outdated
Comment threadext/mysqli/mysqli_api.c Outdated
Comment threadext/mysqli/mysqli_api.c Outdated
Comment threadext/mysqli/mysqli_api.c Outdated
Comment threadext/mysqli/mysqli_api.c Outdated
Comment threadext/mysqli/mysqli_nonapi.c Outdated
Comment threadext/mysqli/mysqli_nonapi.c Outdated
Comment threadext/mysqli/mysqli_nonapi.c Outdated
Comment threadext/mysqli/tests/bug74595.phpt Outdated
@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch from c9b011c to 0c4501dCompareSeptember 14, 2020 16:43
Comment threadext/mysqli/mysqli_nonapi.c Outdated
Comment threadext/mysqli/mysqli_nonapi.c Outdated
Comment threadext/mysqli/mysqli.stub.php Outdated
Comment threadext/mysqli/mysqli_api.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_throw_error(NULL, "Function cannot be used with MYSQL_USE_RESULT");
zend_throw_error(NULL, "mysqli_data_seek() cannot be used with MYSQL_USE_RESULT");

etc.

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 have suggested a PR for this error. I believe the constant should be MYSQLI_USE_RESULT

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.

Right, this should be MYSQLI_USE_RESULT.

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 still think the message I suggested is better "cannot be used in MYSQLI_USE_RESULT mode". The constant describes result mode. You don't use the function with this constant but rather your result is in one or the other mode.

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.

You can address this in your PR, I just want to get done with this one at this point ^^"

@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch from 0c4501d to e096674CompareSeptember 15, 2020 13:52
@nikic

Copy link
Copy Markdown
Member

ext/opcache/tests/func_info.phpt is failing due to outdated zend_func_info.c.

@Girgias
Girgiasforce-pushed the mysqli-warning-promotion branch from e096674 to 8a81c50CompareSeptember 15, 2020 15:53
F1("mysqli_real_escape_string", MAY_BE_STRING),
F1("mysqli_stmt_affected_rows", MAY_BE_LONG | MAY_BE_STRING),
F0("mysqli_stmt_data_seek", MAY_BE_NULL | MAY_BE_FALSE),
F0("mysqli_stmt_data_seek", MAY_BE_NULL),

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 just drop the line completely now.

@Girgias
Girgias deleted the mysqli-warning-promotion branch September 15, 2020 17:14
@GirgiasGirgias mentioned this pull request Sep 15, 2020
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.

5 participants

@Girgias@nikic@kocsismate@kamil-tekiela@carusogabriel