PHPC-1756: Remove oppressive terminology from source code #1221

alcaeus · 2021-05-21T11:52:40Z

The libmongoc update takes care of PHPC-1840 and PHPC-1803. This PR also brings in changes for PHPC-1836 and PHPC-1842.

alcaeus · 2021-05-21T11:53:24Z

php_phongo.c

@@ -1811,19 +1810,6 @@ static bool php_phongo_apply_rp_options_to_uri(mongoc_uri_t* uri, bson_t* option
 	while (bson_iter_next(&iter)) {
 		const char* key = bson_iter_key(&iter);

-		if (!ignore_slaveok && !strcasecmp(key, MONGOC_URI_SLAVEOK)) {


See PHPC-1836

alcaeus · 2021-05-21T11:54:03Z

.gitmodules

@@ -1,6 +1,6 @@
 [submodule "src/libmongoc"]
 	path = src/libmongoc
-	url = https://github.com/mongodb/mongo-c-driver.git
+	url = https://github.com/alcaeus/mongo-c-driver.git


I'll update this PR once all the pull requests in libmongoc have been merged.

The only libmongoc PR I found open is for CDRIVER-3985, which seems unrelated (just internal to libmongoc).

For the two PHPC tickets referenced in this PR that you said are covered by libmongoc:

PHPC-1803 depends on CDRIVER-3947, which is already merged.

PHPC-1840 depends on CDRIVER-3997, which is just "in progress" with no PR.

So is this pointing to your branch for CDRIVER-3997, for which there is no PR yet?

Yes, the change also includes my work done for CDRIVER-3997, which is ready but was already building on 2 PRs that were in review, so I held off on creating the PR for that.

alcaeus · 2021-05-21T11:54:42Z

php_phongo.c

 			zval_ptr_dtor(&state.zchild);
 			return false;
 		}

-		ADD_ASSOC_ZVAL_EX(retval, "last_is_master", &state.zchild);
+		ADD_ASSOC_ZVAL_EX(retval, "last_hello_response", &state.zchild);


Since we never documented the returned fields, I decided to drop the old field entirely. Let me know if you think we should keep it in for now.

This is just used for debug output, since Manager and Server are not serializable. No objections to changing this, since we don't document debug output at all.

jmikola · 2021-05-24T13:59:47Z

.gitmodules

@@ -1,6 +1,6 @@
 [submodule "src/libmongoc"]
 	path = src/libmongoc
-	url = https://github.com/mongodb/mongo-c-driver.git
+	url = https://github.com/alcaeus/mongo-c-driver.git


The only libmongoc PR I found open is for CDRIVER-3985, which seems unrelated (just internal to libmongoc).

For the two PHPC tickets referenced in this PR that you said are covered by libmongoc:

PHPC-1803 depends on CDRIVER-3947, which is already merged.

PHPC-1840 depends on CDRIVER-3997, which is just "in progress" with no PR.

So is this pointing to your branch for CDRIVER-3997, for which there is no PR yet?

jmikola · 2021-05-24T14:10:22Z

php_phongo.c

 			zval_ptr_dtor(&state.zchild);
 			return false;
 		}

-		ADD_ASSOC_ZVAL_EX(retval, "last_is_master", &state.zchild);
+		ADD_ASSOC_ZVAL_EX(retval, "last_hello_response", &state.zchild);


This is just used for debug output, since Manager and Server are not serializable. No objections to changing this, since we don't document debug output at all.

jmikola · 2021-05-24T14:11:36Z

php_phongo.c

@@ -1553,7 +1553,7 @@ static bool php_phongo_apply_options_to_uri(mongoc_uri_t* uri, bson_t* options)
 			!strcasecmp(key, MONGOC_URI_READPREFERENCE) ||
 			!strcasecmp(key, MONGOC_URI_READPREFERENCETAGS) ||
 			!strcasecmp(key, MONGOC_URI_SAFE) ||
-			!strcasecmp(key, MONGOC_URI_SLAVEOK) ||
+			!strcasecmp(key, MONGOC_URI_SECONDARYOK) ||


If slaveOk handling was removed entirely from php_phongo_apply_rp_options_to_uri, what's the reason for renaming it to a new constant here?

This seems like it's related to PHPC-1836 and CDRIVER-3989 (removing slaveOk URI handling), but I don't see this constant in the corresponding libmongoc PR: mongodb/mongo-c-driver#794

I found a reference to this in your cdriver-3997 branch, but it's still defined to "slaveok". Was there a more recent decision to continue parsing the legacy URI option and only rename the constant?

I didn't take a look at libmongoc's URI string parsing (not sure where to look as this work is split between so many tickets), but if PHP needs to explicitly ignore it here that suggests that libmongoc would otherwise still accept it by mongoc_uri_option_is_bool in the code down below.

What's the reason for libmongoc continuing to parse this if we're dropping it entirely in PHP? I realize they might not be able to drop the old constant due to ABI, but I expect they could stop using it and allow slaveOk in a URI string to start raising warnings for unsupported URI options (something we don't surface through PHP).

Yes, this can be removed as libmongoc ignores the option completely

src/MongoDB/Server.c

tests/connect/standalone-ssl-verify_cert-error-001.phpt

tests/manager/manager-set-uri-options-002.phpt

tests/server/server-executeBulkWrite-004.phpt

jmikola · 2021-05-25T15:26:41Z

tests/server/server-executeBulkWrite-004.phpt

 OK: Got MongoDB\Driver\Exception\RuntimeException
-not master
+not (primary|master)


Using a regex within EXPECTF requires wrapping the pattern with %r. I think this might be responsible for the CI test failures.

Oh. I assumed they could be added directly, so I didn't bother to run them. Thanks!

jmikola

LGTM once regex patterns in EXPECTF are fixed.

jmikola · 2021-05-25T15:27:01Z

tests/server/server-executeBulkWrite-007.phpt

 OK: Got MongoDB\Driver\Exception\RuntimeException
-not master
+not (primary|master)


Same comment as https://github.com/mongodb/mongo-php-driver/pull/1221/files#r638907540

alcaeus · 2021-05-26T12:17:08Z

Test failures are due to 5.0 behaving differently. I'm fixing those in PHPC-1848. Waiting for mongodb/mongo-c-driver#797 to be merged before proceeding here.

alcaeus · 2021-06-22T08:57:23Z

@jmikola test failures are being fixed in #1225 - should we merge this and fix the build in your PR?

jmikola

OK to sort out test failures in #1225.

alcaeus requested a review from jmikola May 21, 2021 11:52

alcaeus self-assigned this May 21, 2021

alcaeus commented May 21, 2021

View reviewed changes

alcaeus changed the title ~~PHPC-1841: Remove oppressive terminology from source code~~ PHPC-1842: Remove oppressive terminology from source code May 21, 2021

alcaeus changed the title ~~PHPC-1842: Remove oppressive terminology from source code~~ PHPC-1756: Remove oppressive terminology from source code May 21, 2021

jmikola reviewed May 24, 2021

View reviewed changes

jmikola reviewed May 25, 2021

View reviewed changes

alcaeus mentioned this pull request May 26, 2021

PHPLIB-654 Remove deprecated terminology mongodb/mongo-php-library#828

Merged

alcaeus force-pushed the phpc-1841 branch from 9e888a7 to c1986d5 Compare May 26, 2021 08:28

alcaeus changed the title ~~PHPC-1756: Remove oppressive terminology from source code~~ [WAIT] PHPC-1756: Remove oppressive terminology from source code May 26, 2021

jmikola mentioned this pull request Jun 4, 2021

PHPC-1765 and PHPC-1821: DBRef and dot/dollar validation tests #1225

Merged

alcaeus added 7 commits June 22, 2021 08:34

Use latest version of libmongoc

5ef619e

Remove deprecated terminology for primary

f9f9a0c

Remove deprecated terminology for secondary

4942097

Fix clang-format

9e194b6

Remove check for legacy field

a720428

Fix missing space in error expectation

45948d9

Harden test expectation around primary checks

e685a85

alcaeus force-pushed the phpc-1841 branch from c1986d5 to e685a85 Compare June 22, 2021 06:36

alcaeus changed the title ~~[WAIT] PHPC-1756: Remove oppressive terminology from source code~~ PHPC-1756: Remove oppressive terminology from source code Jun 22, 2021

alcaeus requested a review from jmikola June 22, 2021 08:57

jmikola approved these changes Jun 22, 2021

View reviewed changes

alcaeus merged commit a612c37 into mongodb:master Jun 22, 2021

alcaeus deleted the phpc-1841 branch June 22, 2021 17:31

PHPC-1756: Remove oppressive terminology from source code #1221

PHPC-1756: Remove oppressive terminology from source code #1221

Uh oh!

Conversation

alcaeus commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alcaeus commented Jun 22, 2021

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alcaeus commented May 21, 2021 •

edited

Loading

alcaeus commented May 26, 2021 •

edited

Loading