ext/pgsql: adding pg_set_error_context_visibility. #11395
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
another level of context for pg_last_error/pg_result_error() to include or not the context in those. PQSHOW_CONTEXT_ERRORS being the default.
Girgias
reviewed
Jun 9, 2023
ext/pgsql/pgsql.c
Outdated
Comment on lines
2843
to
2849
| if (ZEND_NUM_ARGS() == 1) { | ||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &visibility) == FAILURE) { | ||
| RETURN_THROWS(); | ||
| } | ||
| link = FETCH_DEFAULT_LINK(); | ||
| CHECK_DEFAULT_LINK(link); | ||
| } else { |
Member
There was a problem hiding this comment.
Considering we have deprecated the fetching of the default link I would rather not introduce a new function with this functionality. Would also make the stubs correct.
ext/pgsql/pgsql.c
Outdated
Comment on lines
2859
to
2863
| if (visibility & (PQSHOW_CONTEXT_NEVER|PQSHOW_CONTEXT_ERRORS|PQSHOW_CONTEXT_ALWAYS)) { | ||
| RETURN_LONG(PQsetErrorContextVisibility(pgsql, visibility)); | ||
| } else { | ||
| RETURN_FALSE; | ||
| } |
Member
There was a problem hiding this comment.
Make this a ValueError in case of a wrong value?
devnexen
force-pushed
the
pgsql_error_update
branch
from
8e15b79 to
de6a771
Compare
Girgias
approved these changes
Jun 13, 2023
Member
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Minor nits but LGTM.
Do not forget to add an entry in UPGRADING about the new function
ext/pgsql/pgsql.stub.php
Outdated
Comment on lines
974
to
975
| /** @param PgSql\Connection|int $connection */ | ||
| function pg_set_error_context_visibility($connection, int $visibility = UNKNOWN): int|false {} |
Member
There was a problem hiding this comment.
Since the visibility is not optional anymore and it doesn't return false on failure.
Suggested change
| /** @param PgSql\Connection|int $connection */ | |
| function pg_set_error_context_visibility($connection, int $visibility = UNKNOWN): int|false {} | |
| function pg_set_error_context_visibility(PgSql\Connection $connection, int $visibility): int {} |
ext/pgsql/tests/07optional.phpt
Outdated
Comment on lines
24
to
28
| if (function_exists('pg_set_error_context_visibility')) { | ||
| pg_set_error_context_visibility($db, PGSQL_SHOW_CONTEXT_NEVER); | ||
| pg_set_error_context_visibility($db, PGSQL_SHOW_CONTEXT_ERRORS); | ||
| pg_set_error_context_visibility($db, PGSQL_SHOW_CONTEXT_ALWAYS); | ||
| } |
Member
There was a problem hiding this comment.
It looks like it's always defined?
devnexen
force-pushed
the
pgsql_error_update
branch
from
de6a771 to
427a632
Compare
Girgias
reviewed
Jun 13, 2023
| function pg_pipeline_status(PgSql\Connection $connection): int {} | ||
| #endif | ||
|
|
||
| /** @param PgSql\Connection|int $connection */ |
Member
There was a problem hiding this comment.
This comment is not needed any more now.
Feel free to remove this when merging if CI is happy.
Member
|
This breaks build on old RHEL 7 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
another level of context for pg_last_error/pg_result_error() to include or not the context in those. PQSHOW_CONTEXT_ERRORS being the default.