Skip to content

PHPC-1925: Change ServerDescription::getType() to return a string #1249

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

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

tanlisu
Copy link
Contributor

@tanlisu tanlisu commented Aug 17, 2021

@tanlisu tanlisu requested a review from jmikola August 17, 2021 17:57
@@ -115,7 +125,7 @@ static PHP_METHOD(ServerDescription, getType)

PHONGO_PARSE_PARAMETERS_NONE();

RETVAL_LONG(php_phongo_server_description_type(intern->server_description));
RETVAL_STRING(mongoc_server_description_type(intern->server_description));
Copy link
Member

Choose a reason for hiding this comment

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

This would be a good opportunity to revise serverDescription-getType-001.phpt similar to what you did for the TopologyDescription::getType() test.

Additionally, php_phongo_serverdescription_get_properties_hash can be changed to output a "type" as a string instead of calling php_phongo_server_description_type. Its test can also be revised to use %r like the corresponding TopologyDescription tests.

@jmikola
Copy link
Member

jmikola commented Aug 17, 2021

According to CI, server-getServerDescription-001.phpt still needs to be updated.

@tanlisu
Copy link
Contributor Author

tanlisu commented Aug 18, 2021

Yes sorry, I saw but didn't have time to finish yesterday

@jmikola
Copy link
Member

jmikola commented Aug 18, 2021

No worries. I wasn't sure if you caught it since it was after the review was requested.

@tanlisu tanlisu merged commit e3d105a into mongodb:feature/sdam-monitoring Aug 18, 2021
@tanlisu tanlisu deleted the phpc-1925 branch August 18, 2021 15:25
jmikola pushed a commit to jmikola/mongo-php-driver that referenced this pull request Jan 10, 2022
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.

2 participants