Skip to content
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

[behat]Extract Request creation to separate service #13896

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

Ferror
Copy link
Contributor

@Ferror Ferror commented Apr 26, 2022

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets none
License MIT

It is a part of improving Developer Experience in Behat tests. I removed factory methods out of the Request class, as the object has really grown up. Moved these methods to separate Factory required a lot of changes, that's why I decided to postpone here and merge current changes.

Created Request Builder and Factory will provide an additional layer for future developers to build their own Requests

src/Sylius/Behat/Client/RequestFactory.php Outdated Show resolved Hide resolved
$builder->withHeader($name, $value);
}

return $builder->build();
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking if we shouldn't return the builder and allow for a build call in the contexts. This would allow for the decoration of this object as well. Perhaps two services would be a good choice? Then we could finalize this service and the default approach to extending it would be with decoration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea, but I would rather stick with the current solution for now, and in another PR return Builder. To achieve that I will need to refactor a lot ApiPlatform Client

Copy link
Member

Choose a reason for hiding this comment

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

We could introduce RequestBuilderFactory which will have almost the same API but will return builder, which will be called in factories. But perhaps I'm making it too complex.

src/Sylius/Behat/Client/RequestFactory.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Client/RequestBuilder.php Outdated Show resolved Hide resolved
@Ferror Ferror changed the title Behat api client dx3 Behat Refactor - Extract Request creation to seperate service Apr 26, 2022
@Ferror Ferror changed the title Behat Refactor - Extract Request creation to seperate service Behat Refactor - Extract Request creation to separate service Apr 26, 2022
@Ferror Ferror marked this pull request as ready for review April 26, 2022 12:05
@Ferror Ferror requested a review from a team as a code owner April 26, 2022 12:05
@Ferror Ferror changed the title Behat Refactor - Extract Request creation to separate service [behat]Extract Request creation to separate service Apr 26, 2022
src/Sylius/Behat/Client/ContentTypeGuide.php Show resolved Hide resolved
Comment on lines +38 to +43
public function withHeader(string $name, string $value): self
{
$this->headers[$name] = $value;

return $this;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if there would be any benefit of making these calls immutable

Suggested change
public function withHeader(string $name, string $value): self
{
$this->headers[$name] = $value;
return $this;
}
public function withHeader(string $name, string $value): self
{
$builder = self::create($this->uri, $this->method);
$builder->headers[$name] = $value;
return $builder;
}

Copy link
Contributor Author

@Ferror Ferror Apr 27, 2022

Choose a reason for hiding this comment

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

I'm thinking that the builder is kind of mutable pattern (but I might be wrong). Your example implementation will fail with the use case:

$builder
  ->withHeader('header1', 'value1')
  ->withHeader('header2', 'value2')

We could change builder implementation like this: (postponed most of the methods)

<?php

final class RequestBuilder
{
    private function __construct(
        private string $uri,
        private string $method,
        private array $parameters = [],
        private array $headers = [],
        private array $content = [],
        private array $files = [],
    )
    {
    }

    public static function create(string $uri, string $method,  /* ... */): self
    {
        return new self($uri, $method);
    }

    public function withHeader(string $name, string $value): self
    {
        $builder = self::create($this->uri, $this->method, $this->parameters, $this->headers /* ... */);
        $builder->headers[$name] = $value;
        
        return $builder;
    }

    public function build(): RequestInterface
    {
        return new Request(
            $this->uri,
            $this->method,
            $this->parameters,
            $this->headers,
            $this->content,
            $this->files,
        );
    }
}

But i don't know if this isn't a little bit of overenginnering

$builder->withHeader($name, $value);
}

return $builder->build();
Copy link
Member

Choose a reason for hiding this comment

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

We could introduce RequestBuilderFactory which will have almost the same API but will return builder, which will be called in factories. But perhaps I'm making it too complex.

@lchrusciel lchrusciel merged commit 3d0c17d into Sylius:master Apr 26, 2022
@lchrusciel
Copy link
Member

Thanks, Zbigniew! 🥇

@GSadee GSadee added the Behat Issues and PRs aimed at improving Behat usage. label Apr 27, 2022
@Ferror Ferror deleted the behat-api-client-dx3 branch April 27, 2022 06:53
GSadee added a commit that referenced this pull request Oct 17, 2022
…eMilek)

This PR was merged into the 1.12 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12 <!-- see the comment below -->                  |
| Bug fix?        | no                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                      |
| Deprecations?   | no <!-- don't forget to update the UPGRADE-*.md file --> |
| Related tickets | mentioned in [#Z](#13896)                      |
| License         | MIT                                                          |

<!--
 - Bug fixes must be submitted against the 1.11 branch
 - Features and deprecations must be submitted against the 1.12 branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

98f4d94 [API][Behat] Add note about behat request changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Behat Issues and PRs aimed at improving Behat usage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants