PHPC-1716 Allow configuring server API version in manager #1204

alcaeus · 2021-03-01T11:39:35Z

This PR is built on the current WIP state of versioned API support in the C driver to allow designing the PHP driver interface and verify that it passes all spec tests. Note that the changes to submodules (including the libmongoc version) will be re-done once versioned API support lands in the main libmongoc development branch.

jmikola · 2021-03-22T17:45:34Z

tests/bson-corpus/array-decodeError-001.phpt

@@ -7,7 +7,7 @@ DO NOT EDIT THIS FILE
 --FILE--
 <?php

-require_once __DIR__ . '/../utils/tools.php';


Is this the result of using a wide net for search/replace? I would have assumed tests that manually include tools.php could be left as-is, because they typically do not connect to a server.

Yes, I replaced all instances of tools.php to require the .inc file for consistency. I can revert this change if you think that's unwise.

It's not necessary but I can see how it'd make maintenance easier by not having to think about which file to include. No objections to leaving as-is.

jmikola · 2021-03-22T17:48:40Z

.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


Is this still necessary given that CDRIVER-3821 is merged?

The only open PR I found on libmongoc is mongodb/mongo-c-driver#753, but that seems unrelated to PHP.

I had not managed to create the PR before. mongodb/mongo-c-driver#765 contains the changes necessary, most notably fixing a wrong function name and adding a getter for the version field in mongoc_server_api_t.

src/MongoDB/ServerApi.c

jmikola · 2021-03-22T18:04:20Z

src/MongoDB/ServerApi.c

+
+	if (*server_api) {
+		mongoc_server_api_destroy(*server_api);
+	}


Is this ever reached? If not, would it be better to throw an exception and return false? AFAICT, this is only called from the constructor and php_phongo_serverapi_init_from_hash, both of which should use freshly initialized objects.

Replaced this with an exception instructing users to file a bug report as it shouldn't happen.

Should we standardize on American English spelling ("initialized") here?

I forgot we use American English as opposed to British English. I'll update it.

src/MongoDB/ServerApi.c

tests/utils/tools.php

.evergreen/config.yml

jmikola

Some questions on code formatting for macros, but LGTM otherwise.

Beyond that, take a look at the CI failures. AppVeyor reports a compilation failure in some macro.

The Evergreen failure appears to be related to what you fixed in #1206.

jmikola · 2021-03-24T20:47:59Z

phongo_compat.h

+			EH_THROW,                                                           \
+			phongo_exception_from_phongo_domain(PHONGO_ERROR_INVALID_ARGUMENT), \
+			&error_handling);                                                   \
+	ZEND_PARSE_PARAMETERS_START(min_num_args, max_num_args)


Should this be indented consistently with the lines above since it falls within do {?

You're right, totally missed that. Thanks!

Ah...I was wondering about this: it seems like clang-format immediately wants to undo my changes and restore this broken formatting, so I'm afraid we're stuck with this for the time being :(

jmikola · 2021-03-24T20:48:14Z

phongo_compat.h

+	ZEND_PARSE_PARAMETERS_START(min_num_args, max_num_args)
+
+#define PHONGO_PARSE_PARAMETERS_END                   \
+	ZEND_PARSE_PARAMETERS_END_EX(                     \


Same question about indentation here.

jmikola · 2021-03-24T20:50:48Z

tests/bson-corpus/array-decodeError-001.phpt

@@ -7,7 +7,7 @@ DO NOT EDIT THIS FILE
 --FILE--
 <?php

-require_once __DIR__ . '/../utils/tools.php';


It's not necessary but I can see how it'd make maintenance easier by not having to think about which file to include. No objections to leaving as-is.

alcaeus · 2021-03-25T12:09:05Z

Beyond that, take a look at the CI failures. AppVeyor reports a compilation failure in some macro.

This was due to me using ZEND_STRL in zend_hash_str_add, which is implemented as a macro in PHP < 7.3. I've removed this usage for the time being. According to @cmb69, ZEND_STRL should not be used; this will most likely also affect other usages of ZEND_STRL in similar instances.

The Evergreen failure appears to be related to what you fixed in #1206.

Yes, those are fixed as well now.

alcaeus · 2021-03-25T12:09:39Z

Note: holding off on merge until mongodb/mongo-c-driver#765 is merged.

jmikola · 2021-03-25T18:13:31Z

This was due to me using ZEND_STRL in zend_hash_str_add, which is implemented as a macro in PHP < 7.3. I've removed this usage for the time being. According to @cmb69, ZEND_STRL should not be used; this will most likely also affect other usages of ZEND_STRL in similar instances.

I'm familiar with that. I assume it's because ZEND_STRL may not be expanded before the preprocessor checks arguments for the outer macro (zend_hash_str_add in this case).

Our previous issues with ZEND_STRL had to do with it only being suitable for literals (see: https://github.com/mongodb/mongo-php-driver/search?q=ZEND_STRL&type=commits). That seems unrelated to this.

I don't think we need to stop using ZEND_STRL altogether, though. The build failures when we do something wrong should be obvious :)

A centralised entry point is required to inject the API_VERSION env variable later.

This changed when migrating from our own scripts to drivers-evergreen-tools and was not updated properly, causing all tests to run with auth disabled.

This macro is missing on PHP < 7.3

This causes compile failures on PHP < 7.3 that I have yet to understand.

alcaeus requested a review from jmikola March 1, 2021 11:39

alcaeus self-assigned this Mar 1, 2021

alcaeus force-pushed the phpc-1716 branch from 0850383 to a40994e Compare March 1, 2021 11:41

alcaeus force-pushed the phpc-1716 branch 3 times, most recently from b241321 to 0248588 Compare March 10, 2021 12:17

alcaeus changed the title ~~[POC] PHPC-1716 Allow configuring server API version in manager~~ PHPC-1716 Allow configuring server API version in manager Mar 10, 2021

alcaeus force-pushed the phpc-1716 branch 3 times, most recently from 851de9e to bac6f18 Compare March 22, 2021 17:17

jmikola reviewed Mar 22, 2021

View reviewed changes

jmikola approved these changes Mar 24, 2021

View reviewed changes

alcaeus force-pushed the phpc-1716 branch from 4bd4657 to ef70e28 Compare March 25, 2021 09:34

alcaeus changed the title ~~PHPC-1716 Allow configuring server API version in manager~~ [WAIT] PHPC-1716 Allow configuring server API version in manager Mar 25, 2021

alcaeus mentioned this pull request Mar 26, 2021

PHPLIB-622 Versioned MongoDB API for Drivers mongodb/mongo-php-library#816

Merged

alcaeus added 12 commits March 26, 2021 11:01

Introduce MongoDB\Driver\ServerApi

6c0dbfe

Accept serverApi driver option

e8652f2

Introduce create_test_manager factory to create manager

58ef8f6

A centralised entry point is required to inject the API_VERSION env variable later.

Add build variant to test with requireApiVersion=true

8266bc0

Fix wrong configuration for auth variable

2c8de0c

This changed when migrating from our own scripts to drivers-evergreen-tools and was not updated properly, causing all tests to run with auth disabled.

Declare ZEND_PARSE_PARAMETERS_NONE macro

a88f3c3

This macro is missing on PHP < 7.3

Remove duplicated API param storage

bc14872

Add missing semicolons

62b5bfd

Add ZEND_PARSE_PARAMETERS_NON_EX macro

d1b2a7a

Extract error handling functionality to separate macros

5b25a60

Throw if internal mongoc_server_api_t is already initialised

579712b

Use imported namespaces in tools file

b8b4e34

alcaeus added 5 commits March 26, 2021 11:01

Fix type info for reflection

d7a33d2

Use American English spelling

3258ec7

Only use typed serialize signature on PHP 8+

7beb720

Update PHONGO_PARSE_PARAMETERS_NONE macro for PHP < 7.3

349e8c6

Remove usage of ZEND_STRL within zend_hash_str_add

8eb4a30

This causes compile failures on PHP < 7.3 that I have yet to understand.

alcaeus force-pushed the phpc-1716 branch from 527dfbc to 60f4b53 Compare March 26, 2021 12:21

alcaeus changed the title ~~[WAIT] PHPC-1716 Allow configuring server API version in manager~~ PHPC-1716 Allow configuring server API version in manager Mar 26, 2021

Fix errors in new PHONGO_PARSE_PARAMETERS macros

5606fd6

alcaeus force-pushed the phpc-1716 branch from 60f4b53 to 5606fd6 Compare March 26, 2021 13:03

alcaeus merged commit deead96 into mongodb:master Mar 26, 2021

alcaeus deleted the phpc-1716 branch March 26, 2021 14:43

PHPC-1716 Allow configuring server API version in manager #1204

PHPC-1716 Allow configuring server API version in manager #1204

Uh oh!

Conversation

alcaeus commented Mar 1, 2021

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmikola left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

alcaeus commented Mar 25, 2021

Uh oh!

alcaeus commented Mar 25, 2021

Uh oh!

jmikola commented Mar 25, 2021

Uh oh!

Uh oh!

alcaeus Mar 24, 2021 •

edited

Loading

alcaeus Mar 25, 2021 •

edited

Loading

jmikola left a comment •

edited

Loading