Skip to content
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

Revisions to ext-mongodb architecture and library tutorial #2688

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Aug 15, 2023

@jmikola jmikola requested a review from alcaeus August 15, 2023 18:45
Remove references to HHVM, the legacy mongo extension, and older PHP versions from the architecture article and component diagram.

Replace the original diagram with an SVG graphic.

https://jira.mongodb.org/browse/PHPC-2276
Copy link
Member Author

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

cc: @GromNaN

as the <code>mongodb</code> extension.
This article explains how all the different components of the PHP driver fit
together, from base system libraries, through the extension, and to the PHP
libraries on top.
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, this article was originally adapted from a blog post. Much of that copy is no longer relevant to the audience, so I've removed references to the legacy extension API.

In addition to removing HHVM and PHP 5.x from the architecture diagram, I added libmongocrypt as a third system library dependency.

@@ -153,7 +106,8 @@
options). The driver will attempt to find a previously persisted
<link xlink:href="&url.mongodb.libmongoc;">libmongoc</link> client object for
that hash. If an existing client cannot be found for the hash, a new client
will be created (and persisted for future use).
will be created and persisted for future use. This behavior can be disabled
via the <literal>"disableClientPersistence"</literal> driver option.
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 isn't related to PHPC-2276, but it came up while reading through the same source file. IIRC we originally decided not to discuss this option anywhere other than the constructor docs, so I'm amenable to reverting this.

While re-reading this with fresh eyes, I just didn't like that the paragraph was so authoritative about clients always being persisted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Explaining the full behaviour here makes sense 👍

<imageobject>
<imagedata fileref="en/reference/mongodb/images/driver_arch.png"/>
<imagedata fileref="en/reference/mongodb/images/driver_arch.svg" width="625" depth="450" format="SVG" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced the original PNG file with an SVG. There was prior art for this in the PHP manual:

doc-en/faq/passwords.xml

Lines 163 to 172 in 272838c

<mediaobject>
<alt>
The components of the value returned by password_hash and crypt: in
order, the chosen algorithm, the algorithm's options, the salt used,
and the hashed password.
</alt>
<imageobject>
<imagedata fileref="en/faq/figures/crypt-text-rendered.svg" width="690" depth="192" format="SVG" />
</imageobject>
</mediaobject>

Rendered: https://www.php.net/manual/en/faq.passwords.php#faq.password.storing-salts

@jmikola jmikola changed the title PHPC-2276: Revise Architecture Overview article in docs Revisions to MongoDB driver architecture and library tutorials Aug 15, 2023
@jmikola jmikola changed the title Revisions to MongoDB driver architecture and library tutorials Revisions to ext-mongodb architecture and library tutorial Aug 15, 2023
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.

Just nits to make the work of translation slightly easier

reference/mongodb/architecture.xml Outdated Show resolved Hide resolved
reference/mongodb/architecture.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Thanks @jmikola!

Co-authored-by: George Peter Banyard <[email protected]>
@jmikola jmikola merged commit 077667a into php:master Aug 18, 2023
2 checks passed
@jmikola jmikola deleted the phpc-2276 branch August 18, 2023 16:13
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