PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server::getServerDescription() #1239

tanlisu · 2021-07-19T21:54:29Z

https://jira.mongodb.org/browse/PHPC-1805
https://jira.mongodb.org/browse/PHPC-1910

config.m4

config.w32

php_phongo.c

php_phongo_classes.h

php_phongo_structs.h

src/MongoDB/Server.c

src/MongoDB/ServerDescription.c

tests/server/server-getServerDescription-001.phpt

src/MongoDB/ServerDescription.c

tests/serverDescription/serverDescription-getType-001.phpt

src/MongoDB/ServerDescription.c

tanlisu · 2021-07-28T17:21:30Z

The GitHub CI test failure for the PHP 8.1 build environment (https://github.com/mongodb/mongo-php-driver/pull/1239/checks?check_run_id=3183644508) is expected I believe - see https://jira.mongodb.org/browse/PHPC-1849

jmikola · 2021-07-28T19:00:03Z

The GitHub CI test failure for the PHP 8.1 build environment

Please rebase on (or merge in) master to pull in #1241. I think it makes sense to just disable 8.1 until we address PHPC-1849.

php_phongo.c

jmikola

Some comments on the code. I'll get to the tests afterwards when those points are addressed and I run this locally.

src/MongoDB/Server.c

src/MongoDB/ServerDescription.c

jmikola

Should be good to go after this round of feedback is addressed.

jmikola · 2021-07-30T15:17:33Z

src/MongoDB/ServerDescription.c

+
+	helloResponse = mongoc_server_description_hello_response(intern->server_description);
+
+	php_phongo_bson_state state;


For consistency we should declare all variables at the top of the scope. Perhaps you don't see this using clang on macOS, but I get the following error compiling with GCC:

src/MongoDB/ServerDescription.c: In function ‘zim_ServerDescription_getHelloResponse’: src/MongoDB/ServerDescription.c:44:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]

Note that newer C standards would permit this (see: https://stackoverflow.com/q/8474100).

I'm not sure why this wasn't caught by CI. Looking at the build status for the PR, only AppVeyor (Windows) appears to have run on the most recent iteration.

src/MongoDB/ServerDescription.c

jmikola · 2021-07-30T15:31:43Z

tests/serverDescription/serverDescription-debug-001.phpt

+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
+<?php skip_if_not_live(); ?>
+<?php skip_if_not_clean(); ?>


IIRC, this is only needed if the test is going to read/write data. Since you're just selecting a server here, we don't need to concern ourselves with the collection under test being empty.

Ok, the same SKIPIF is included in server-debug.phpt - should I make a ticket to remove that?

You can just edit the file in GitHub and generate a PR (instead of directly committing). I don't think we need to track that with a ticket.

Same comment as #1242 (comment)

I was only suggesting that skip_if_not_clean be removed.

tests/serverDescription/serverDescription-debug-001.phpt

tests/serverDescription/serverDescription-getType-001.phpt

tests/serverDescription/serverDescription-var_export-001.phpt

tests/serverDescription/serverDescription-getType-001.phpt

jmikola · 2021-07-30T17:53:18Z

I'm not sure why this wasn't caught by CI. Looking at the build status for the PR, only AppVeyor (Windows) appears to have run on the most recent iteration.

I think this is due to the GitHub CI config only expecting certain branches for PRs (see: tests.yml). I just addressed that in 69cc2b5 and merged it into the feature/sdam-monitoring branch. Please rebase and force-push this again to try and trigger a CI build.

The Evergreen config likely has something similar, but it's less flexible as I think we'd need an entirely different project created just for the feature branch. We can do without that for now.

tanlisu · 2021-07-30T18:22:06Z

I rebased on the feature/sdam-monitoring branch but I think the GitHub CI is still missing the Coding Standards test.

jmikola · 2021-07-30T19:36:51Z

I rebased on the feature/sdam-monitoring branch but I think the GitHub CI is still missing the Coding Standards test.

I didn't realize we had multiple YML configs. Fixed in f3bf5a3 and just merged into the feature branch.

…ents

Co-authored-by: Andreas Braun <alcaeus@users.noreply.github.com>

This partially reverts commit 27dde83 due to build failures.

Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

jmikola · 2021-07-30T21:25:23Z

The PR description had a reference to PHPC-1849, which I think was incorrect. I changed that to PHPC-1910.

Also, one of the commits says "Implement ServerDescription class and add to monitoring events". Since this PR didn't actually integrate anything with the APM events please make sure that's removed from the message when merging. In fact, I'm not sure any of the individual commit messages need to be kept when squashing, since they don't have any relevance beyond the code review. The PR title sufficiently describes everything in this case.

…::getServerDescription() (mongodb#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

…::getServerDescription() (#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

…::getServerDescription() (mongodb#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

…::getServerDescription() (#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

…::getServerDescription() (mongodb#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

tanlisu requested a review from alcaeus July 19, 2021 21:57

alcaeus reviewed Jul 20, 2021

View reviewed changes

tanlisu commented Jul 20, 2021

View reviewed changes

src/MongoDB/ServerDescription.c Outdated Show resolved Hide resolved

tanlisu force-pushed the phpc-1805 branch 2 times, most recently from 5bb2146 to a8ec9e7 Compare July 23, 2021 20:36

jmikola reviewed Jul 27, 2021

View reviewed changes

src/MongoDB/ServerDescription.c Outdated Show resolved Hide resolved

jmikola reviewed Jul 27, 2021

View reviewed changes

tests/serverDescription/serverDescription-getType-001.phpt Outdated Show resolved Hide resolved

tanlisu force-pushed the phpc-1805 branch from e5cf895 to 12c64f1 Compare July 28, 2021 15:45

tanlisu commented Jul 28, 2021

View reviewed changes

src/MongoDB/ServerDescription.c Outdated Show resolved Hide resolved

tanlisu marked this pull request as ready for review July 28, 2021 16:07

tanlisu force-pushed the phpc-1805 branch from c2c41ed to 2ee222d Compare July 28, 2021 18:38

tanlisu force-pushed the phpc-1805 branch from 2ee222d to 676f5ca Compare July 28, 2021 19:06

tanlisu changed the title ~~PHPC-1805: Implement ServerDescription class and add to monitoring ev…~~ PHPC-1805 and PHPC-1849: Implement ServerDescription class and Server::getServerDescription() Jul 28, 2021

jmikola changed the base branch from master to feature/sdam-monitoring July 29, 2021 14:40

jmikola reviewed Jul 29, 2021

View reviewed changes

php_phongo.c Outdated Show resolved Hide resolved

jmikola reviewed Jul 29, 2021

View reviewed changes

tanlisu force-pushed the phpc-1805 branch from 90b3ad4 to d7bde2f Compare July 29, 2021 19:58

jmikola reviewed Jul 30, 2021

View reviewed changes

tanlisu force-pushed the phpc-1805 branch from b886770 to 1f85854 Compare July 30, 2021 17:38

tanlisu force-pushed the phpc-1805 branch from 1f85854 to d01c69b Compare July 30, 2021 18:13

jmikola mentioned this pull request Jul 30, 2021

Clean up server-debug.phpt #1242

Merged

tanlisu and others added 4 commits July 30, 2021 15:57

PHPC-1805: Implement ServerDescription class and add to monitoring ev…

b78e499

…ents

Update config.m4

29dac21

Co-authored-by: Andreas Braun <alcaeus@users.noreply.github.com>

Apply suggestions from code review

1effcea

Co-authored-by: Andreas Braun <alcaeus@users.noreply.github.com>

Apply suggestions from code review

947493c

tanlisu and others added 17 commits July 30, 2021 15:57

Add all ServerDescription functions

3c1d295

Add ServerDescription debug test

9a991ed

Revert "Apply suggestions from code review"

ead8372

This partially reverts commit 27dde83 due to build failures.

Fix build

d55b224

Fix build failures

4e2bbc2

Fix function header comments

19f8a7b

Update ServerDescription tests

cfc199a

Fix mixed declarations and code error, clean up

d40bca1

Update getTags() tests

f7ff681

Update ServerDescription functions and tests

31214e2

Improve ServerDescription::getType() test

07219e9

Update Server::getServerDescription() test

99e85f0

Fix build error

5435d0a

Update ServerDescription get_properties_hash()

c0a9355

Remove php_phongo_server_description_to_zval()

f54c3d7

Apply suggestions from code review

995a850

Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

Update ServerDescription tests

966dd92

tanlisu force-pushed the phpc-1805 branch from d01c69b to 966dd92 Compare July 30, 2021 20:07

jmikola changed the title ~~PHPC-1805 and PHPC-1849: Implement ServerDescription class and Server::getServerDescription()~~ PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server::getServerDescription() Jul 30, 2021

jmikola approved these changes Jul 30, 2021

View reviewed changes

tanlisu merged commit 693e264 into mongodb:feature/sdam-monitoring Jul 30, 2021

jmikola added a commit that referenced this pull request Nov 3, 2021

PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server…

f9643ec

…::getServerDescription() (#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

jmikola added a commit that referenced this pull request Dec 30, 2021

PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server…

1b5c445

…::getServerDescription() (#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

jmikola added a commit that referenced this pull request Jan 5, 2022

PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server…

9ed2baf

…::getServerDescription() (#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server::getServerDescription() #1239

PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server::getServerDescription() #1239

tanlisu commented Jul 19, 2021 •

edited by jmikola

tanlisu commented Jul 28, 2021

jmikola commented Jul 28, 2021

jmikola left a comment

jmikola left a comment

jmikola Jul 30, 2021 •

edited

jmikola Jul 30, 2021

tanlisu Jul 30, 2021

jmikola Jul 30, 2021

tanlisu Jul 30, 2021

jmikola Jul 30, 2021

jmikola commented Jul 30, 2021

tanlisu commented Jul 30, 2021

jmikola commented Jul 30, 2021

jmikola commented Jul 30, 2021


		helloResponse = mongoc_server_description_hello_response(intern->server_description);

		php_phongo_bson_state state;

PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server::getServerDescription() #1239

PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server::getServerDescription() #1239

Conversation

tanlisu commented Jul 19, 2021 • edited by jmikola

tanlisu commented Jul 28, 2021

jmikola commented Jul 28, 2021

jmikola left a comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

jmikola Jul 30, 2021 • edited

Choose a reason for hiding this comment

jmikola Jul 30, 2021

Choose a reason for hiding this comment

tanlisu Jul 30, 2021

Choose a reason for hiding this comment

jmikola Jul 30, 2021

Choose a reason for hiding this comment

tanlisu Jul 30, 2021

Choose a reason for hiding this comment

jmikola Jul 30, 2021

Choose a reason for hiding this comment

jmikola commented Jul 30, 2021

tanlisu commented Jul 30, 2021

jmikola commented Jul 30, 2021

jmikola commented Jul 30, 2021

tanlisu commented Jul 19, 2021 •

edited by jmikola

jmikola Jul 30, 2021 •

edited