SearchForm#set: return self

[tremby]

This is is a fix to various methods of SearchForm to return the current instance rather than a new one, as per its own documentation.

The bug report I was going to submit follows:

---

I'm building up a search form, predicate by predicate. I like to do this on separate lines of code, since that makes it easy to wrap some lines in conditionals, for example.

I got caught up just now because the query method returns a new instance rather than modifying and returning itself, due to the set method doing that.

Presuming I do want to split it into separate lines, instead of being able to do

$form = $api->forms()->everything;
$form->ref($api->master());
$form->query(Predicate::....);
$form->query(Predicate::...);
$result = $form->submit();

I have to do

$form = $api->forms()->everything;
$form = $form->ref($api->master());
$form = $form->query(Predicate::...);
$form = $form->query(Predicate::...);
$result = $form->submit();

which seems very awkward.

Even the documentation of $searchForm->set() (which both of these methods use) suggests it modifies the existing object, but in fact it doesn't. https://github.com/prismicio/php-kit/blob/master/src/Prismic/SearchForm.php#L66

    /**
     * Sets a value for a given parameter. For instance: set('orderings', '[product.price]'),
     * or set('page', 2).
     *
     * Checks that the parameter is expected in the RESTful form before allowing to add it.
     *
     * @api
     * @param  string $key the name of the parameter
     * @param  string $value the value of the parameter
     * @throws \RuntimeException
     * @return \Prismic\SearchForm the current SearchForm object, with the new parameter added
     */

"return the current SearchForm object" -- this is not what it does. It'd be a lot more useful if it did.

Incidentally, there's a case where currently it returns null. I wonder if this should in fact be throwing an exception or returning the unmodified $this.

[tremby]

Let me know if I should rebase this on master. Current master is broken for me, so I branched off of the latest release which is working for me.

[erwan]

Hi,

Thank you for your contribution, however the current behaviour is immutable on purpose, so you can keep a partially built SearchForm and reuse it for various queries.

Typically you can do this:

$form = $api->forms()->everything->ref($api->master());
$result = $form->query(Predicate::foo)->submit();
$result2 = $form->query(Predicate::bar)->submit();

That would not be possible if SearchForm becomes mutable.

I recommend to keep chaining as a single call, that you can still break before the -> for lisibility and only go for the more verbose $form = $form-> when you do need to put a conditional.

[tremby]

@erwan, I think this is a bad decision.

PHP has the clone keyword which can be used to make copies of the form in its partial state. To keep code fluent, it would be trivial to implement __clone and to make the clone as deep as necessary, then to add a helper clone method to invoke the clone keyword and return the copy. To do the equivalent of your example you would then write

$form = $api->forms()->everything->ref($api->master());
$result = $form->clone()->query(Predicate::foo)->submit();
$result2 = $form->clone()->query(Predicate::bar)->submit();

This would make for a more consistent and useful API, easily used in both ways. In my experience a wide range of popular APIs use this fluent approach, including PHP's built in DateTime, as well as Monolog, Symfony's Console module, Swiftmailer, certain parts of Laravel, Carbon, the AWS SDK, Mockery... And then jQuery and the like.

But if you choose to stick with your decision, you still need to fix the documentation and implementations to stay consistent and describe the actual functionality. The documentation for various methods suggests to me that the object is modified and then returned, not that a new one is returned. I can see how it could be interpreted the other way, so at best it is ambiguous. And now at present depending on the arguments some operations return a new object while others modify it and return the modified original, which is leading in my opinion to a very confusing API.

• SeachForm#set says @return \Prismic\SearchForm the current SearchForm object, with the new parameter added, which as mentioned above is ambiguous at best. The method sometimes returns the current object and sometimes returns a new one, depending on the parameters.
• SearchForm#ref has similar documentation and returns the result of SearchForm#set, so it suffers from the same problem.
• Same with SearchForm#after.
• Same with SearchForm#fetch.
• Same with SearchForm#pageSize.
• Same with SearchForm#page.
• Same with SearchForm#orderings.
• Same with SearchForm#fetchLinks (documentation here says @return \Prismic\SearchForm the current SearchForm object, with the new pageSize parameter added which is presumably a copy-and-paste bug).
• Same with SearchForm#query (documentation here says @return \Prismic\SearchForm the current SearchForm object, with the new page parameter added which is presumably another copy-and-paste bug).

[tremby]

On reflection, I take back "bad decision", and I see it as just a difference of opinion. I do think the documentation should be improved, however. I'll likely put in a pull request tomorrow.