-
Notifications
You must be signed in to change notification settings - Fork 812
Syn docs with ibm_db2 stubs #2316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@NattyNarwhal could you review this as you seems to be the maintainer of the extension currently. It would probably make a lot of sense to add various XML entities in language-snippets to prevent the needless repetitions for standard parameters across functions. |
@@ -12,7 +12,7 @@ | |||
&reftitle.description; | |||
<methodsynopsis> | |||
<type>string</type><methodname>db2_stmt_error</methodname> | |||
<methodparam choice="opt"><type>resource</type><parameter>stmt</parameter></methodparam> | |||
<methodparam choice="opt"><type>resource</type><parameter>stmt</parameter><initializer>&null;</initializer></methodparam> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kocsismate seems like the gen_stub script doesn't take into account implicit nullability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Girgias IIRC implicit nullability is not supported. I.e. you should get Parameter $varName has null default, but is not nullable
exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I didn't really get any issues from generating the method synopsis via gen_stub. But I suppose this is something that needs patching upstream then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Girgias, do you have a suggestion about how to represent optional parameters in a stub that don't have a default value in the underlying implementation?
Currently, the stub is using null
as default value for them (even when it is not true). Maybe there is a better way to represent that situation.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I proposed this as a possible "convention".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I answered here: php/pecl-database-ibm_db2#32 (comment)
Discussion upstream about the autocommit stuff: php/pecl-database-ibm_db2#32 |
Discussion in the linked issue brought up gen-stub compatibility - will generated headers work on older PHP 8.x, even if it has newer constructs (for documentation and such) that appear in newer PHP? I know about the legacy arginfo header for 7.x, but I don't know what guarantees are present for 8.x. That'd affect what constructs we can put into the stub, and thus seemingly the documentation? |
@NattyNarwhal gen_stub.php available in PHP 8.2 takes version compatibility into account: php/php-src#8931 |
@Girgias, the stub |
13f6c76
to
41481ea
Compare
@phansys just pulled the latest stub changes and regenerated the method synopsis. Does it look good to you now? |
<methodparam><type>string</type><parameter>table-name</parameter></methodparam> | ||
<methodparam><type class="union"><type>string</type><type>null</type></type><parameter>qualifier</parameter></methodparam> | ||
<methodparam><type class="union"><type>string</type><type>null</type></type><parameter>schema</parameter></methodparam> | ||
<methodparam><type>string</type><parameter>table_name</parameter></methodparam> | ||
<methodparam><type>bool</type><parameter>unique</parameter></methodparam> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for this parameter should also be updated, as it currently mentions integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay done, the weird values is because it's initially some C int constants.
Hopefully this is now ready to be merged. |
ups forgot about this |
* Sync with latest stub * Update db2_statistics() unique param description
Supersedes #2300.
Generated using:
php ../../php-src/build/gen_stub.php --replace-methodsynopses ../../ibm_db2/ibm_db2.stub.php ./reference/ibm_db2/