Skip to content

OCI8 class renaming for PHP 8#237

Closed
cjbj wants to merge 1 commit intophp:masterfrom
cjbj:oci8classes
Closed

OCI8 class renaming for PHP 8#237
cjbj wants to merge 1 commit intophp:masterfrom
cjbj:oci8classes

Conversation

@cjbj
Copy link
Copy Markdown
Contributor

@cjbj cjbj commented Nov 25, 2020

This is the change discussed on the doc mail list. Untested for various reasons. Let's see if it builds.
It has the id renames, since that's what I currently have along with the other changes.

Copy link
Copy Markdown
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! :)

&reftitle.description;
<methodsynopsis role="oop">
<type class="union"><type>string</type><type>false</type></type><methodname>OCI-Lob::read</methodname>
<type>string</type><methodname>OCILob::read</methodname>
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.

According to the return values section, this method returns false on failure; that should either be reflected in the in the methodsynopsis, or get fixed in the return values section. If that change is only relevant as of PHP 8.0.0, it could be listed as a changelog entry.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is because in between me initially working on the changes, you had corrected the synopsis. I will pick up your changes.

Comment on lines +15 to +19
<note>
<para>
The OCI-Collection class was renamed to OCICollection in PHP 8 and PECL OCI8 3.0.0 to align with PHP naming standards.
</para>
</note>
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 would prefer to have changelog entries instead:

Suggested change
<note>
<para>
The OCI-Collection class was renamed to OCICollection in PHP 8 and PECL OCI8 3.0.0 to align with PHP naming standards.
</para>
</note>
<refsect1 role="changelog">
&reftitle.changelog;
<informaltable>
<tgroup cols="2">
<thead>
<row>
<entry>&Version;</entry>
<entry>&Description;</entry>
</row>
</thead>
<tbody>
<row>
<entry>8.0.0, PECL OCI8 3.0.0</entry>
<entry>
The <classname>OCI-Collection</classname> class was renamed to
<classname>OCICollection</classname> to align with PHP naming standards.
</entry>
</row>
</tbody>
</tgroup>
</informaltable>
</refsect1>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems logical. It's easy in the individual method files which already <refentry> sections but would require a bit of a rewrite for the OCI-Collection.xml and OCI-Lob.xml; I will defer that nicety.

<row>
<entry>ociassignelem</entry>
<entry><link linkend="oci-collection.assignelem">OCICollection::assignElem</link></entry>
<entry><link linkend="ocicollection.assignelem">OCICollection::assignElem</link></entry>
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'm not 100% convinced that changing the IDs and renaming files is a good idea right now. While it is certainly correct, it causes more work for the translation teams, and these are likely to be under pressure anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

@cjbj
Copy link
Copy Markdown
Contributor Author

cjbj commented Nov 27, 2020

I'm going to update from a reset repo; and will abandon this PR.

@cjbj cjbj closed this Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants