Remove X-Records header #314

bhelx · 2017-05-16T22:43:36Z

This is for API v2.6.

The X-Records header was removed on all GET index endpoints. You must
now call HEAD explicitly to get the header and thus the count.

The consequence of this is that the Recurly_Pager#count method has changed. If the pager has a URL, it will send a HEAD request to get the count of all records. Otherwise it'll return the count of the cached _objects.

$accounts = Recurly_AccountList::get();
// Calling count will make a HEAD request to the server on each call
print $accounts->count() . "\n"; // 1033
print $accounts->count() . "\n"; // 1033

drewish · 2017-05-17T19:26:23Z

Tests/Recurly/Pager_Test.php

@@ -69,8 +69,6 @@ public function testFromHref() {
    $pager->_loadFrom($url);

    $this->assertEquals($url, $pager->getHref());
-    $this->assertEquals(6, $pager->count(), 'Returns correct count');
-    $this->assertEquals(6, count($pager), 'Returns correct count');


I think we still want to keep this interface and we should just update the tests accordingly

these should definitely come back.

drewish · 2017-05-17T19:29:11Z

lib/recurly/pager.php

-        // the same time.
-        $this->_loadFrom($this->_href);
-      }
+  public function count($client = null) {


I don't think the count method should take a client. That should be included in the constructor when the pager is instantiated. We should only be injecting it into static methods.

drewish · 2017-05-17T19:30:50Z

lib/recurly/pager.php

@@ -45,7 +48,7 @@ public function rewind() {
  public function current()
  {
    // Work around pre-PHP 5.5 issue that prevents `empty($this->count())`:
-    if ($this->count() == 0) {
+    if (count($this->_objects) == 0) {


Seems like if empty($this->_objects) would be shorter. And I think then we can also ditch the comment above.

bhelx · 2017-05-22T21:27:32Z

Tests/Recurly/Client_Test.php

@@ -8,7 +8,7 @@ public function testDeprecationError() {
    $this->client->addResponse('GET', '/accounts', 'client/deprecated-200.xml');

    // This should print an error but not raise.
-    $accounts = Recurly_AccountList::get(null, $this->client)->count();
+    $accounts = Recurly_AccountList::get(null, $this->client)->get();


Changed this to get() because the mock client is expecting a GET.

bhelx · 2017-05-22T21:28:01Z

Tests/Recurly/MeasuredUnit_List_Test.php

@@ -9,6 +9,5 @@ public function testGetMeasuredUnits() {
    $measured_units = Recurly_MeasuredUnitList::get(null, $this->client);

    $this->assertInstanceOf('Recurly_MeasuredUnitList', $measured_units);
-    $this->assertEquals(2, $measured_units->count());


I suppose it might be okay to put some of these back?

Yeah I'd like to keep them if we can. At least for the embedded/nested ones that don't require a separate request.

actually i don't think we'll want to check the count, we should just check the href like we do on the other pager tests.

drewish · 2017-05-22T21:49:32Z

Tests/Recurly/Pager_Test.php

@@ -69,8 +69,6 @@ public function testFromHref() {
    $pager->_loadFrom($url);

    $this->assertEquals($url, $pager->getHref());
-    $this->assertEquals(6, $pager->count(), 'Returns correct count');
-    $this->assertEquals(6, count($pager), 'Returns correct count');


these should definitely come back.

drewish · 2017-05-22T21:50:02Z

Tests/Recurly/Pager_Test.php

@@ -81,8 +79,6 @@ public function testFromStub() {
    $this->assertInstanceOf('Mock_Pager', $pager);

    $this->assertEquals($url, $pager->getHref());
-    $this->assertEquals($pager->count(), 6, 'Returns correct count');
-    $this->assertEquals(count($pager), 6, 'Returns correct count');


these should also come back. we can stub in the correctly sized response.

drewish · 2017-05-22T21:50:32Z

Tests/Recurly/Pager_Test.php

+    $this->client->addResponse('HEAD', '/mocks', 'pager/head-200.xml');
+    $pager = new Mock_Pager('/mocks', $this->client);
+    $this->assertEquals($pager->count(), 42);
+  }


I don't think we need this one, it should be covered by the other three scenarios.

drewish · 2017-05-22T21:51:24Z

Tests/fixtures/client/deprecated-200.xml

@@ -6,7 +6,6 @@ Content-Language: en-US
 X-RateLimit-Limit: 2000
 X-RateLimit-Remaining: 1998
 X-RateLimit-Reset: 1467912540
-X-Records: 3


Pretty sure X-Api-Version: 1.999 would still have the X-Records header ;)

drewish · 2017-05-22T21:52:21Z

lib/recurly/base.php

@@ -30,6 +30,21 @@ public static function _get($uri, $client = null)
  }

  /**
+   * Send a HEAD request to the URI, validate the response and return the headers.
+   * @param string Resource URI, if not fully qualified, the base URL will be appended


Nit pick (I get that we're following the existing comment convention): should be "prepended" rather than "appended".

drewish · 2017-05-22T21:53:00Z

lib/recurly/pager.php

-   * Number of records in this list.
+   * Number of records in this list. This calls
+   * the server with a HEAD request every time the function
+   * is called.


The line wrapping seems odd here.

drewish · 2017-05-22T21:54:35Z

lib/recurly/pager.php

@@ -45,8 +46,8 @@ public function rewind() {
  public function current()
  {
    // Work around pre-PHP 5.5 issue that prevents `empty($this->count())`:
-    if ($this->count() == 0) {
-      return null;
+    if (isset($this->_objects) == 0) {


What is the == 0 doing here?

Seems like it could be !isset(?

bhelx · 2017-06-02T18:41:20Z

Tests/Recurly/Pager_Test.php

    $this->assertIteratesCorrectly($pager, 6);
+    $this->assertEquals(2, $pager->count(), 'Returns correct count');
+-   $this->assertEquals(2, count($pager), 'Returns correct count');


^ weird that this - didn't cause any problems

The X-Records header was removed on all GET index endpoints. You must now call HEAD explicitly to get the header and thus the count.

bhelx · 2017-06-02T19:55:48Z

Finally 🍰

bhelx added the WIP label May 16, 2017

drewish reviewed May 17, 2017

View reviewed changes

bhelx removed the WIP label May 22, 2017

bhelx commented May 22, 2017

View reviewed changes

drewish reviewed May 22, 2017

View reviewed changes

bhelx force-pushed the remove_x_records_header branch from 224e362 to 60f5a45 Compare May 31, 2017 22:57

bhelx commented Jun 2, 2017

View reviewed changes

drewish force-pushed the remove_x_records_header branch 3 times, most recently from 661cda6 to 2c43a70 Compare June 2, 2017 19:16

Remove X-Records header

1dfaf59

The X-Records header was removed on all GET index endpoints. You must now call HEAD explicitly to get the header and thus the count.

bhelx force-pushed the remove_x_records_header branch from 2c43a70 to 1dfaf59 Compare June 2, 2017 19:21

drewish approved these changes Jun 2, 2017

View reviewed changes

drewish merged commit 8ffa389 into api_version_2_6 Jun 2, 2017

drewish deleted the remove_x_records_header branch June 2, 2017 19:25

bhelx mentioned this pull request Jun 2, 2017

API Version 2.6 #317

Merged

bhelx added the V2 V2 Client label Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove X-Records header #314

Remove X-Records header #314

bhelx commented May 16, 2017 •

edited

Loading

drewish May 17, 2017

drewish May 22, 2017

drewish May 17, 2017

drewish May 17, 2017

bhelx May 22, 2017

bhelx May 22, 2017

drewish May 22, 2017 •

edited

Loading

drewish Jun 2, 2017

drewish May 22, 2017

drewish May 22, 2017

drewish May 22, 2017

drewish May 22, 2017

bhelx May 22, 2017

drewish May 22, 2017

drewish May 22, 2017

drewish May 22, 2017

drewish May 23, 2017

bhelx Jun 2, 2017

bhelx commented Jun 2, 2017

Remove X-Records header #314

Remove X-Records header #314

Conversation

bhelx commented May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewish May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhelx commented Jun 2, 2017

bhelx commented May 16, 2017 •

edited

Loading

drewish May 22, 2017 •

edited

Loading