Skip to content

Commit 12baa9f

Browse files
tanlisutanlisu
committed
Apply suggestions from code review
1 parent 2bf64a9 commit 12baa9f

11 files changed

+140
-151
lines changed

‎php_phongo.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1285,7 +1285,10 @@ bool php_phongo_topology_description_to_zval(zval* retval, mongoc_topology_descr
12851285
}
12861286

12871287
add_next_index_zval(&servers, &obj);
1288+
zval_ptr_dtor(&obj);
1289+
zval_ptr_dtor(&servers);
12881290
}
1291+
mongoc_server_descriptions_destroy_all(sds, n);
12891292

12901293
ADD_ASSOC_ZVAL_EX(retval, "servers", &servers);
12911294
}

‎src/MongoDB/Monitoring/TopologyChangedEvent.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ void php_phongo_topologychangedevent_init_ce(INIT_FUNC_ARGS) /* {{{ */
162162
memcpy(&php_phongo_handler_topologychangedevent, phongo_get_std_object_handlers(), sizeof(zend_object_handlers));
163163
php_phongo_handler_topologychangedevent.get_debug_info = php_phongo_topologychangedevent_get_debug_info;
164164
php_phongo_handler_topologychangedevent.free_obj = php_phongo_topologychangedevent_free_object;
165-
php_phongo_handler_topologychangedevent.offset = XtOffsetOf(php_phongo_commandstartedevent_t, std);
165+
php_phongo_handler_topologychangedevent.offset = XtOffsetOf(php_phongo_topologychangedevent_t, std);
166166

167167
return;
168168
} /* }}} */

‎src/MongoDB/TopologyDescription.c

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,9 @@ static PHP_METHOD(TopologyDescription, getServers)
5353
for (i = 0; i < n; i++) {
5454
zval obj;
5555

56-
php_phongo_server_description_to_zval(&obj, sds[i]);
56+
phongo_serverdescription_init(&obj, sds[i]);
5757
add_next_index_zval(return_value, &obj);
5858
}
59-
60-
mongoc_server_descriptions_destroy_all(sds, n);
6159
} /* }}} */
6260

6361
/* {{{ proto boolean MongoDB\Driver\TopologyDescription::hasReadableServer([?MongoDB\Driver\ReadPreference $readPreference])
@@ -72,7 +70,7 @@ static PHP_METHOD(TopologyDescription, hasReadableServer)
7270

7371
PHONGO_PARSE_PARAMETERS_START(0, 1)
7472
Z_PARAM_OPTIONAL
75-
Z_PARAM_ZVAL_EX(z_read_preference, 1, 0)
73+
Z_PARAM_OBJECT_OF_CLASS(z_read_preference, php_phongo_readpreference_ce)
7674
PHONGO_PARSE_PARAMETERS_END();
7775

7876
if (z_read_preference) {
@@ -105,19 +103,23 @@ static PHP_METHOD(TopologyDescription, getType)
105103
} /* }}} */
106104

107105
/* {{{ MongoDB\Driver\TopologyDescription function entries */
106+
/* clang-format off */
107+
ZEND_BEGIN_ARG_INFO_EX(ai_TopologyDescription_hasReadableServer, 0, 0, 0)
108+
ZEND_ARG_OBJ_INFO(0, readPreference, MongoDB\\Driver\\ReadPreference, 1)
109+
ZEND_END_ARG_INFO()
110+
108111
ZEND_BEGIN_ARG_INFO_EX(ai_TopologyDescription_void, 0, 0, 0)
109112
ZEND_END_ARG_INFO()
110113

111114
static zend_function_entry php_phongo_topologydescription_me[] = {
112-
/* clang-format off */
113115
PHP_ME(TopologyDescription, getServers, ai_TopologyDescription_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
114-
PHP_ME(TopologyDescription, hasReadableServer, ai_TopologyDescription_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
116+
PHP_ME(TopologyDescription, hasReadableServer, ai_TopologyDescription_hasReadableServer, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
115117
PHP_ME(TopologyDescription, hasWritableServer, ai_TopologyDescription_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
116118
PHP_ME(TopologyDescription, getType, ai_TopologyDescription_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
117119
ZEND_NAMED_ME(__construct, PHP_FN(MongoDB_disabled___construct), ai_TopologyDescription_void, ZEND_ACC_PRIVATE | ZEND_ACC_FINAL)
118120
PHP_FE_END
119-
/* clang-format on */
120121
};
122+
/* clang-format on */
121123
/* }}} */
122124

123125
/* {{{ MongoDB\Driver\TopologyDescription object handlers */

‎tests/apm/monitoring-topologyChanged-001.phpt

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ MongoDB\Driver\Monitoring\TopologyChangedEvent
33
--SKIPIF--
44
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
55
<?php skip_if_not_live(); ?>
6-
<?php skip_if_not_clean(); ?>
76
--FILE--
87
<?php
98
require_once __DIR__ . "/../utils/basic.inc";
@@ -16,27 +15,28 @@ class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
1615

1716
public function topologyChanged(MongoDB\Driver\Monitoring\TopologyChangedEvent $event)
1817
{
19-
if (! $this->topologyDescription) {
20-
echo "- getTopologyId() returns an object: ", is_object( $event->getTopologyId() ) ? 'yes' : 'no', "\n";
21-
22-
$this->topologyDescription = $event->getNewDescription();
23-
echo "- topologyDescription->getType() type returns a string: ", is_string( $this->topologyDescription->getType() ) ? 'yes' : 'no', "\n";
24-
echo "- topologyDescription->getServers() returns an array: ", is_array( $this->topologyDescription->getServers() ) ? 'yes' : 'no', "\n";
18+
if (isset($this->topologyDescription)) {
19+
return;
2520
}
21+
22+
$this->topologyDescription = $event->getNewDescription();
23+
echo "- getTopologyId() returns an ObjectId: ", ($event->getTopologyId() instanceof MongoDB\BSON\ObjectId) ? 'yes' : 'no', "\n";
24+
echo "- getNewDescription() returns a TopologyDescription: ", ($event->getNewDescription() instanceof MongoDB\Driver\TopologyDescription) ? 'yes' : 'no', "\n";
25+
echo "- getPreviousDescription() returns a TopologyDescription: ", ($event->getPreviousDescription() instanceof MongoDB\Driver\TopologyDescription) ? 'yes' : 'no', "\n";
2626
}
2727
}
2828

2929
$subscriber = new MySubscriber;
30-
MongoDB\Driver\Monitoring\addSubscriber( $subscriber );
30+
MongoDB\Driver\Monitoring\addSubscriber($subscriber);
3131

3232
$command = new MongoDB\Driver\Command(['ping' => 1]);
3333
$m->executeCommand(DATABASE_NAME, $command);
3434

3535
?>
3636
===DONE===
3737
<?php exit(0); ?>
38-
--EXPECTF--
39-
- getTopologyId() returns an object: yes
40-
- topologyDescription->getType() type returns a string: yes
41-
- topologyDescription->getServers() returns an array: yes
38+
--EXPECT--
39+
- getTopologyId() returns an ObjectId: yes
40+
- getNewDescription() returns a TopologyDescription: yes
41+
- getPreviousDescription() returns a TopologyDescription: yes
4242
===DONE===

‎tests/topologyDescription/topologyDescription-debug-001.phpt

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,32 +7,40 @@ MongoDB\Driver\TopologyDescription debug output
77
<?php
88
require_once __DIR__ . "/../utils/basic.inc";
99

10-
$manager = create_test_manager();
11-
12-
class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
10+
class TopologyDescriptionProvider implements MongoDB\Driver\Monitoring\SDAMSubscriber
1311
{
1412
private $topologyDescription;
1513

1614
public function topologyChanged(MongoDB\Driver\Monitoring\TopologyChangedEvent $event)
1715
{
18-
if (! $this->topologyDescription) {
19-
$this->topologyDescription = $event->getNewDescription();
20-
var_dump($this->topologyDescription);
21-
}
16+
$this->topologyDescription = $event->getNewDescription();
2217
}
23-
}
2418

25-
$subscriber = new MySubscriber;
26-
$manager->addSubscriber($subscriber);
19+
public function getTopologyDescription()
20+
{
21+
$manager = create_test_manager();
22+
$manager->addSubscriber($this);
23+
$manager->executeCommand(DATABASE_NAME, new MongoDB\Driver\Command(['ping' => 1]));
24+
$manager->removeSubscriber($this);
25+
26+
return $this->topologyDescription;
27+
}
28+
}
2729

28-
$command = new MongoDB\Driver\Command(['ping' => 1]);
29-
$manager->executeCommand(DATABASE_NAME, $command);
30+
$subscriber = new TopologyDescriptionProvider;
31+
$topologyDescription = $subscriber->getTopologyDescription();
32+
var_dump($topologyDescription);
3033

3134
?>
3235
===DONE===
3336
<?php exit(0); ?>
3437
--EXPECTF--
3538
object(MongoDB\Driver\TopologyDescription)#%d (%d) {
36-
%a
39+
["servers"]=>
40+
array(%d) {
41+
%a
42+
}
43+
["type"]=>
44+
string(%d) "%rSingle|Sharded|ReplicaSetWithPrimary%r"
3745
}
3846
===DONE===

‎tests/topologyDescription/topologyDescription-getServers-001.phpt

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,31 +7,44 @@ MongoDB\Driver\TopologyDescription::getServers()
77
<?php
88
require_once __DIR__ . "/../utils/basic.inc";
99

10-
$manager = create_test_manager();
11-
12-
class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
10+
class TopologyDescriptionProvider implements MongoDB\Driver\Monitoring\SDAMSubscriber
1311
{
1412
private $topologyDescription;
1513

1614
public function topologyChanged(MongoDB\Driver\Monitoring\TopologyChangedEvent $event)
1715
{
18-
if (! $this->topologyDescription) {
19-
$this->topologyDescription = $event->getNewDescription();
20-
var_dump($this->topologyDescription->getServers());
16+
$this->topologyDescription = $event->getNewDescription();
17+
}
18+
19+
public function getTopologyDescription()
20+
{
21+
$manager = create_test_manager();
22+
$manager->addSubscriber($this);
23+
$manager->executeCommand(DATABASE_NAME, new MongoDB\Driver\Command(['ping' => 1]));
24+
$manager->removeSubscriber($this);
25+
26+
return $this->topologyDescription;
27+
}
28+
}
29+
30+
function assertIsServerDescriptionsArray(array $sds) {
31+
foreach ($sds as $sd) {
32+
if (! $sd instanceof MongoDB\Driver\ServerDescription) {
33+
return false;
2134
}
2235
}
36+
return true;
2337
}
2438

25-
$subscriber = new MySubscriber;
26-
$manager->addSubscriber($subscriber);
39+
$subscriber = new TopologyDescriptionProvider;
40+
$topologyDescription = $subscriber->getTopologyDescription();
41+
$serverDescriptions = $topologyDescription->getServers();
2742

28-
$command = new MongoDB\Driver\Command(['ping' => 1]);
29-
$manager->executeCommand(DATABASE_NAME, $command);
43+
var_dump(assertIsServerDescriptionsArray($serverDescriptions));
3044

3145
?>
3246
===DONE===
3347
<?php exit(0); ?>
34-
--EXPECTF--
35-
array(%d) {%A
36-
}
48+
--EXPECT--
49+
bool(true)
3750
===DONE===

‎tests/topologyDescription/topologyDescription-getType-001.phpt

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,38 +7,33 @@ MongoDB\Driver\TopologyDescription::getType()
77
<?php
88
require_once __DIR__ . "/../utils/basic.inc";
99

10-
$manager = create_test_manager();
11-
12-
class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
10+
class TopologyDescriptionProvider implements MongoDB\Driver\Monitoring\SDAMSubscriber
1311
{
1412
private $topologyDescription;
1513

1614
public function topologyChanged(MongoDB\Driver\Monitoring\TopologyChangedEvent $event)
1715
{
18-
$expected_types = array(
19-
MongoDB\Driver\TopologyDescription::TYPE_UNKNOWN,
20-
MongoDB\Driver\TopologyDescription::TYPE_SINGLE,
21-
MongoDB\Driver\TopologyDescription::TYPE_SHARDED,
22-
MongoDB\Driver\TopologyDescription::TYPE_REPLICA_SET_NO_PRIMARY,
23-
MongoDB\Driver\TopologyDescription::TYPE_REPLICA_SET_WITH_PRIMARY
24-
);
16+
$this->topologyDescription = $event->getNewDescription();
17+
}
2518

26-
if (! $this->topologyDescription) {
27-
$this->topologyDescription = $event->getNewDescription();
28-
var_dump(in_array($this->topologyDescription->getType(), $expected_types));
29-
}
19+
public function getTopologyDescription()
20+
{
21+
$manager = create_test_manager();
22+
$manager->addSubscriber($this);
23+
$manager->executeCommand(DATABASE_NAME, new MongoDB\Driver\Command(['ping' => 1]));
24+
$manager->removeSubscriber($this);
25+
26+
return $this->topologyDescription;
3027
}
3128
}
3229

33-
$subscriber = new MySubscriber;
34-
$manager->addSubscriber($subscriber);
35-
36-
$command = new MongoDB\Driver\Command(['ping' => 1]);
37-
$manager->executeCommand(DATABASE_NAME, $command);
30+
$subscriber = new TopologyDescriptionProvider;
31+
$topologyDescription = $subscriber->getTopologyDescription();
32+
var_dump($topologyDescription->getType());
3833

3934
?>
4035
===DONE===
4136
<?php exit(0); ?>
4237
--EXPECTF--
43-
bool(true)
38+
string(%d) "%rSingle|Sharded|ReplicaSetWithPrimary%r"
4439
===DONE===

‎tests/topologyDescription/topologyDescription-hasReadableServer-001.phpt

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,30 +7,33 @@ MongoDB\Driver\TopologyDescription::hasReadableServer()
77
<?php
88
require_once __DIR__ . "/../utils/basic.inc";
99

10-
$manager = create_test_manager();
11-
12-
class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
10+
class TopologyDescriptionProvider implements MongoDB\Driver\Monitoring\SDAMSubscriber
1311
{
1412
private $topologyDescription;
1513

1614
public function topologyChanged(MongoDB\Driver\Monitoring\TopologyChangedEvent $event)
1715
{
18-
if (! $this->topologyDescription) {
19-
$this->topologyDescription = $event->getNewDescription();
20-
var_dump($this->topologyDescription->hasReadableServer());
21-
}
16+
$this->topologyDescription = $event->getNewDescription();
2217
}
23-
}
2418

25-
$subscriber = new MySubscriber;
26-
$manager->addSubscriber($subscriber);
19+
public function getTopologyDescription()
20+
{
21+
$manager = create_test_manager();
22+
$manager->addSubscriber($this);
23+
$manager->executeCommand(DATABASE_NAME, new MongoDB\Driver\Command(['ping' => 1]));
24+
$manager->removeSubscriber($this);
25+
26+
return $this->topologyDescription;
27+
}
28+
}
2729

28-
$command = new MongoDB\Driver\Command(['ping' => 1]);
29-
$manager->executeCommand(DATABASE_NAME, $command);
30+
$subscriber = new TopologyDescriptionProvider;
31+
$topologyDescription = $subscriber->getTopologyDescription();
32+
var_dump($topologyDescription->hasReadableServer());
3033

3134
?>
3235
===DONE===
3336
<?php exit(0); ?>
34-
--EXPECTF--
35-
bool(%s)
37+
--EXPECT--
38+
bool(true)
3639
===DONE===

‎tests/topologyDescription/topologyDescription-hasReadableServer-002.phpt

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,40 @@
11
--TEST--
2-
MongoDB\Driver\TopologyDescription::hasReadableServer()
2+
MongoDB\Driver\TopologyDescription::hasReadableServer() with ReadPreference argument
33
--SKIPIF--
44
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
55
<?php skip_if_not_live(); ?>
66
--FILE--
77
<?php
88
require_once __DIR__ . "/../utils/basic.inc";
99

10-
$manager = create_test_manager();
11-
12-
class MySubscriber implements MongoDB\Driver\Monitoring\SDAMSubscriber
10+
class TopologyDescriptionProvider implements MongoDB\Driver\Monitoring\SDAMSubscriber
1311
{
1412
private $topologyDescription;
1513

1614
public function topologyChanged(MongoDB\Driver\Monitoring\TopologyChangedEvent $event)
1715
{
18-
if (! $this->topologyDescription) {
19-
$this->topologyDescription = $event->getNewDescription();
20-
$rp = new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_PRIMARY);
21-
var_dump($this->topologyDescription->hasReadableServer($rp));
22-
}
16+
$this->topologyDescription = $event->getNewDescription();
2317
}
24-
}
2518

26-
$subscriber = new MySubscriber;
27-
$manager->addSubscriber($subscriber);
19+
public function getTopologyDescription()
20+
{
21+
$manager = create_test_manager();
22+
$manager->addSubscriber($this);
23+
$manager->executeCommand(DATABASE_NAME, new MongoDB\Driver\Command(['ping' => 1]));
24+
$manager->removeSubscriber($this);
25+
26+
return $this->topologyDescription;
27+
}
28+
}
2829

29-
$command = new MongoDB\Driver\Command(['ping' => 1]);
30-
$manager->executeCommand(DATABASE_NAME, $command);
30+
$subscriber = new TopologyDescriptionProvider;
31+
$topologyDescription = $subscriber->getTopologyDescription();
32+
$rp = new MongoDB\Driver\ReadPreference('primary');
33+
var_dump($topologyDescription->hasReadableServer($rp));
3134

3235
?>
3336
===DONE===
3437
<?php exit(0); ?>
35-
--EXPECTF--
36-
bool(%s)
38+
--EXPECT--
39+
bool(true)
3740
===DONE===

0 commit comments

Comments
 (0)