Skip to content

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Sep 27, 2024

@jmikola jmikola requested review from alcaeus and GromNaN September 27, 2024 18:40
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Some markup comments

Comment on lines 1 to 4
<?xml version="1.0" encoding="utf-8"?>
<!-- $Revision$ -->

<refentry xml:id="mongodb-bson-packedarray.fromjson" xmlns="http://docbook.org/ns/docbook" xmlns:xlink="http://www.w3.org/1999/xlink">
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please don't put a blank line in new files :)

Suggested change
<?xml version="1.0" encoding="utf-8"?>
<!-- $Revision$ -->
<refentry xml:id="mongodb-bson-packedarray.fromjson" xmlns="http://docbook.org/ns/docbook" xmlns:xlink="http://www.w3.org/1999/xlink">
<?xml version="1.0" encoding="utf-8"?>
<!-- $Revision$ -->
<refentry xml:id="mongodb-bson-packedarray.fromjson" xmlns="http://docbook.org/ns/docbook" xmlns:xlink="http://www.w3.org/1999/xlink">

Copy link
Member Author

@jmikola jmikola Sep 27, 2024

Choose a reason for hiding this comment

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

Noted! I wasn't aware of that convention. Out of curiosity, is it mentioned anywhere on http://doc.php.net/tutorial/?

@Girgias: Also, I noticed you didn't suggest removing blank lines between <refsect1> blocks. Are those still permitted for readability?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah between <refsect1> is OK. I don't think it is mentioned in the tutorial as that hasn't been updated in a while :/

&reftitle.errors;
<simplelist>
&mongodb.throws.argumentparsing;
<member>Throws <classname>MongoDB\Driver\Exception\UnexpectedValueException</classname> if <parameter>json</parameter> is not a valid array.</member>
Copy link
Member

Choose a reason for hiding this comment

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

Possible mistake?

Suggested change
<member>Throws <classname>MongoDB\Driver\Exception\UnexpectedValueException</classname> if <parameter>json</parameter> is not a valid array.</member>
<member>Throws <classname>MongoDB\Driver\Exception\UnexpectedValueException</classname> if <parameter>json</parameter> is invalid JSON.</member>

Copy link
Member Author

Choose a reason for hiding this comment

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

This was adapted from the Document::fromJSON() method, which says "is not a valid document". I'll change both to say "if $json cannot be parsed as a document" (or "an array").

It's not just a matter of the JSON being valid. PackedArray::fromJSON() requires an actual array (i.e. outermost brackets) or an object with sequential numeric keys. There's some more context in mongodb/mongo-php-driver#1703.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

XML changes look good to me :)

@jmikola jmikola merged commit 3c84b02 into php:master Sep 30, 2024
2 checks passed
@jmikola jmikola deleted the phpc-2350 branch September 30, 2024 13:12
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.

3 participants