Add full batch support without BC breaks #713

teldosas · 2016-12-24T18:51:27Z

I made an initial attempt to add full batch support without the BC breaks that #706 introduced. Awaiting review @yguedidi @SammyK . :) I will adjust the docs too in the following days. :)

yguedidi · 2016-12-29T10:36:19Z

src/Facebook/FacebookBatchRequest.php

     *
     * @param FacebookRequest|array $request
-     * @param string|null           $name
+     * @param string|null|array     $optionsOrName When it is a string it is the name of the request


I'd name it just $options and describe it like "array of batch request options (e.g. ...). If a string is given, it will be the value of the name option." or something like that :)

yguedidi · 2016-12-29T10:44:10Z

src/Facebook/FacebookBatchRequest.php

+            return $this;
+        }
+
+        if (null === $optionsOrName) {


you must check for this one before !is_array($optionsOrName). Currently you will never enter this case

yguedidi · 2016-12-29T10:46:50Z

src/Facebook/FacebookBatchRequest.php

@@ -85,17 +87,35 @@ public function add($request, $name = null)
            throw new \InvalidArgumentException('Argument for add() must be of type array or FacebookRequest.');
        }

+        if (!is_array($optionsOrName)) {
+            $this->add($request, ['name' => $optionsOrName]);


I'd just change the $options value here: $options = ['name' => $options];

yguedidi · 2016-12-29T10:47:06Z

src/Facebook/FacebookBatchRequest.php

+        }
+
+        if (null === $optionsOrName) {
+            $this->add($request, []);


I'd just change the $options value here: $options = [];

yguedidi · 2016-12-29T10:49:05Z

src/Facebook/FacebookBatchRequest.php

-     * @param string|null     $requestName   The name of the request.
-     * @param string|null     $attachedFiles Names of files associated with the request.
+     * @param FacebookRequest       $request                The request entity to convert.
+     * @param string|null|array     $requestNameOrOptions   If it is a string it is the name of the request


same here, $options is enough

yguedidi · 2016-12-29T10:50:11Z

src/Facebook/FacebookBatchRequest.php

+            return $this->requestEntityToBatchArray($request, ['name' => $requestNameOrOptions], $attachedFiles);
+        }
+
+        if (null === $requestNameOrOptions) {


same here, check for this one before !is_array(...)

yguedidi · 2016-12-29T10:50:49Z

src/Facebook/FacebookBatchRequest.php

    {
+
+        if (!is_array($requestNameOrOptions)) {
+            return $this->requestEntityToBatchArray($request, ['name' => $requestNameOrOptions], $attachedFiles);


same here, just change the $options value

yguedidi · 2016-12-29T10:50:51Z

src/Facebook/FacebookBatchRequest.php

+        }
+
+        if (null === $requestNameOrOptions) {
+            return $this->requestEntityToBatchArray($request, [], $attachedFiles);


same here, just change the $options value

teldosas · 2016-12-29T15:06:30Z

I made the changes :) @yguedidi

yguedidi

I'd add a testBatchRequestsWithNameGetConvertedToAnArray test :)
I think you can add the doc now! Thanks

yguedidi · 2016-12-29T15:20:48Z

src/Facebook/FacebookBatchRequest.php

+     * @param FacebookRequest       $request        The request entity to convert.
+     * @param string|null|array     $options        Array of batch request options e.g. 'name', 'omit_response_on_success'.
+     *                                              If a string is given, it is the value of the 'name' option.
+     * @param string|null           $attachedFiles  Names of files associated with the request.


extra spaces, each "column" should be separated by only one space :)

yguedidi · 2016-12-29T15:21:42Z

src/Facebook/FacebookBatchRequest.php

+            $options = [];
+        }
+
+        if (!is_array($options)) {


I'd use elseif here to avoid this if in case $options was null and it become an empty array

teldosas · 2016-12-29T15:42:17Z

I'd add a testBatchRequestsWithNameGetConvertedToAnArray test :)

@yguedidi Don't these two tests (and a few others)

testAddingRequestsWillBeFormattedInAnArrayProperly and
testBatchRequestEntitiesProperlyGetConvertedToAnArray
cover this already? They do use the add method with a string value. :)

yguedidi · 2016-12-29T16:08:27Z

@teldosas right! :)

teldosas · 2016-12-29T16:23:16Z

@yguedidi Ok then I made the last changes too. I'll add the docs asap :)

teldosas · 2016-12-31T00:09:39Z

@yguedidi I added the docs. It should be ready now :)

localheinz · 2017-01-06T23:14:06Z

docs/examples/batch_request.md

+
+```php
+$fb = new Facebook\Facebook([
+  'app_id' => '{app-id}',


The indentation here and in the other PHP code examples in this file seems to be off, can we consistently use PSR-2 here instead and indent with 4 spaces?

teldosas · 2017-01-12T20:46:14Z

I fixed the spaces in the examples too @localheinz

teldosas · 2017-01-16T14:18:42Z

@yguedidi @SammyK this should be ready to merge :)

SammyK · 2017-01-16T21:31:43Z

Thanks @teldosas! I'll let @yguedidi give the final review and we'll pull it in. :)

yguedidi

@SammyK if it looks good to you, you can merge and release :)
Thanks a lot @teldosas!

teldosas · 2017-01-18T00:11:28Z

@yguedidi It's been my pleasure 😄 Now that I think of it again couldn't we have just overloaded the add method to avoid BC? I think it would be cleaner.

SammyK · 2017-01-19T15:53:23Z

Sounds good! I'll try to give this a more detailed look tomorrow. I'm curious about the status of #706. I might be able to cherry-pick this one over to master and we could create a new PR with the cleaner API for v6. Thoughts? :)

teldosas · 2017-01-19T15:57:12Z

@SammyK #706 was approved by @yguedidi before we decide to start a PR with no BCs. What if after you cherry pick this to master we resolve any conflicts with #706 and merge it?

localheinz · 2017-01-28T00:56:20Z

docs/examples/batch_request.md

+```php
+<?php
+$fb = new Facebook\Facebook([
+  'app_id' => '{app-id}',


Indentation is still off, can we adjust?

localheinz · 2017-01-28T00:56:37Z

docs/examples/batch_request.md

+```php
+<?php
+$fb = new Facebook\Facebook([
+  'app_id' => '{app-id}',


Indentation is still off, can we adjust?

localheinz · 2017-01-28T00:56:49Z

docs/reference/Facebook.md

+## newBatchRequest()
+```php
+public Facebook\FacebookBatchRequest newBatchRequest(
+  string|AccessToken|null $accessToken,


Indentation is still off, can we adjust?

localheinz · 2017-01-28T00:56:58Z

docs/reference/FacebookBatchRequest.md

@@ -62,12 +63,12 @@ Since the `Facebook\FacebookBatchRequest` is extended from the [`Facebook\Facebo
 ```php
 public add(
  array|Facebook\FacebookBatchRequest $request,


Indentation is still off, can we adjust?

teldosas · 2017-01-28T12:36:47Z

@localheinz done!

SammyK · 2017-04-20T14:02:26Z

Thanks so much for your hard work on this @teldosas! Feel free to send a new PR to the master branch with this feature without worrying about BC if you want to. :)

Add FacebookBatchRequest factory method

5de90f6

teldosas force-pushed the full-batch-v5.4 branch 2 times, most recently from 4257f97 to c1cc66a Compare December 24, 2016 18:55

facebook-github-bot added the CLA Signed label Dec 24, 2016

teldosas mentioned this pull request Dec 24, 2016

Implement omit_response_on_success batch option #706

Closed

yguedidi suggested changes Dec 29, 2016

View reviewed changes

teldosas force-pushed the full-batch-v5.4 branch from c1cc66a to 1be7009 Compare December 29, 2016 15:04

yguedidi suggested changes Dec 29, 2016

View reviewed changes

teldosas added 3 commits December 29, 2016 17:32

Add full batch support without breaking BC

eeab858

Add test for FacebookBatchRequest factory method

27e959b

Add tests for omit_response_on_success batch option

f9e7d99

teldosas force-pushed the full-batch-v5.4 branch from 1be7009 to f9e7d99 Compare December 29, 2016 16:18

teldosas force-pushed the full-batch-v5.4 branch from 17c5bb4 to f319b4c Compare December 31, 2016 00:07

localheinz suggested changes Jan 6, 2017

View reviewed changes

teldosas force-pushed the full-batch-v5.4 branch from f319b4c to 217daea Compare January 12, 2017 20:44

yguedidi approved these changes Jan 17, 2017

View reviewed changes

localheinz reviewed Jan 28, 2017

View reviewed changes

localheinz suggested changes Jan 28, 2017

View reviewed changes

Add docs for batch request factory method

db7cb87

teldosas added 4 commits January 28, 2017 14:33

Update docs of FacebookBatchRequest::add method

177b2e9

Add example for 'omit_response_on_success' option

7f8685f

Add example for 'depends_on' option

16a6a0c

Fix batch examples to comply with PSR2

4980f3a

teldosas force-pushed the full-batch-v5.4 branch from 217daea to 4980f3a Compare January 28, 2017 12:36

SammyK merged commit ecab68b into facebookarchive:5.4 Apr 20, 2017

SammyK mentioned this pull request Apr 20, 2017

[6.0] Batch Request support for omit_response_on_success #438

Closed

Add full batch support without BC breaks #713

Add full batch support without BC breaks #713

Uh oh!

Conversation

teldosas commented Dec 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

teldosas commented Dec 29, 2016

Uh oh!

yguedidi left a comment

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!

teldosas commented Dec 29, 2016

Uh oh!

yguedidi commented Dec 29, 2016

Uh oh!

teldosas commented Dec 29, 2016

Uh oh!

teldosas commented Dec 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teldosas commented Jan 12, 2017

Uh oh!

teldosas commented Jan 16, 2017

Uh oh!

SammyK commented Jan 16, 2017

Uh oh!

yguedidi left a comment

Choose a reason for hiding this comment

Uh oh!

teldosas commented Jan 18, 2017

Uh oh!

SammyK commented Jan 19, 2017

Uh oh!

teldosas commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

teldosas commented Jan 28, 2017

Uh oh!

SammyK commented Apr 20, 2017

Uh oh!

Uh oh!

teldosas commented Dec 24, 2016 •

edited

Loading

teldosas commented Jan 19, 2017 •

edited

Loading