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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version 1.0.0 Changes #6

Closed
allejo opened this issue Jan 8, 2019 · 14 comments
Closed

Version 1.0.0 Changes #6

allejo opened this issue Jan 8, 2019 · 14 comments
Labels

Comments

@allejo
Copy link
Owner

@allejo allejo commented Jan 8, 2019

So this project has apparently become useful to others too. Cool! 馃帀

I originally tagged this project as 0.0.x because I intended things to change and didn't want to surprise people, so I'm hoping semver will convey that message. Nothing major will be changing, but I would like some change with the configuration method.

Configuration Method

In order to configure the latest master branch, you feed in an array. However, the names of these options were chosen when the project only modified Request information. As of #5, modifying Response information will be available too meaning the current names can get confusing.

VCRCleaner::enable(array(
    'ignoreHostname'      => false,
    'ignoreUrlParameters' => array(),
    'ignoreHeaders'       => array(),
    'bodyScrubbers'       => array(),
    'ignoreResponseHeaders' => array(),
    'responseBodyScrubbers' => array(),
));

I'd like to propose the following structure for configuring this project in 1.0.x:

VCRCleaner::enable(array(
    'request' => array(
        'ignoreHostname'    => boolean,
        'ignoreQueryFields' => string[],
        'ignoreHeaders'     => string[],
        'bodyScrubbers'     => Array<(string $body): string>
    ),
    'response' => array(
        'ignoreHeaders'     => string[],
        'bodyScrubbers'     => Array<(string $body): string>
    ),
));

Since no names will conflict between the new structure and the old structure, a backwards-compatible method could exist, but I don't feel it's necessary for some simply renaming of array keys.

New Features in 1.0.0

  • Ability to hide the hostname of requests via request.ignoreHostname
  • Ability to sanitize response headers and body

Project Internals

  • Minor refactoring with how the configuration options are stored/used
  • Minor cleanup of code that can be improved

I'm going to nag you both, @janvernieuwe and @pfuhrmann, for your thoughts on this since you two are the only ones I've seen on this repo. Feel free to invite ours to give feedback.

@allejo allejo added the discussion label Jan 8, 2019
@allejo allejo pinned this issue Jan 8, 2019
@pfuhrmann

This comment has been minimized.

Copy link
Contributor

@pfuhrmann pfuhrmann commented Jan 8, 2019

This is amazing! I totally agree with the new structure of the config.

Only thing I can mention is when redacting hostname you are adding [redacted] string which is not consistent with the rest of the sanitizing. Maybe we should just remove hostname string to keep it consistent with ignoreHeaders and ignoreQueryFields and change the config to ignoreHostname?

@allejo

This comment has been minimized.

Copy link
Owner Author

@allejo allejo commented Jan 8, 2019

Only thing I can mention is when redacting hostname you are adding [redacted] string which is not consistent with the rest of the sanitizing. Maybe we should just remove hostname string to keep it consistent with ignoreHeaders and ignoreQueryFields and change the config to ignoreHostname?

Ohh you bring up a good point! I like much ignoreHostname better.

So the reason I'm replacing the hostname with [redacted] is because if I strip it completely from the URL, then parse_url() will fail. To be consistent with ignoreHeaders, I could change [redacted] with null instead?

$u = parse_url('https://example.com/api?per_page=25&page=1');
print_r($u);

// Array
// (
//     [scheme] => https
//     [host] => example.com
//     [path] => /api
//     [query] => per_page=25&page=1
// )
$u = parse_url('https:///api?per_page=25&page=1'); // this will fail
print_r($u);

Speaking on consistency, ignoreHeaders will replace the header values with null in the cassette but ignoreQueryFields will remove the value from the URL entirely. Do you think this should change?

@pfuhrmann

This comment has been minimized.

Copy link
Contributor

@pfuhrmann pfuhrmann commented Jan 8, 2019

What do you think about the following format?

https://[]/api?per_page=25&page=1

It is still going to parse and perhaps more indicative of "ignored" hostname.

@allejo

This comment has been minimized.

Copy link
Owner Author

@allejo allejo commented Jan 8, 2019

+1 for [], I like how it reads.

Should ignoreHostname remain a boolean or should it be ignoreHostnames instead and it'd only ignore certain hostnames that you specify?

@pfuhrmann

This comment has been minimized.

Copy link
Contributor

@pfuhrmann pfuhrmann commented Jan 8, 2019

I would say to keep it simple. Unless you need that functionality, I am fine with just having simple boolean to hide it all.

@janvernieuwe

This comment has been minimized.

Copy link
Contributor

@janvernieuwe janvernieuwe commented Jan 9, 2019

Just wondering, why not take this opportunity to change to a value object for configuration?

@allejo

This comment has been minimized.

Copy link
Owner Author

@allejo allejo commented Jan 9, 2019

@janvernieuwe how do you imagine that looking?

@janvernieuwe

This comment has been minimized.

Copy link
Contributor

@janvernieuwe janvernieuwe commented Jan 9, 2019

Something like this (very high level):

class RequestOptions
{
    private $ignoreHostname = false;
    private $ignoreQueryFields = [];
    // Add rest of properties

    public function withIgnoreHostname(): RequestOptions
    {
        $new = clone $this;
        $new->ignoreHostname = true;

        return $new;
    }

    public function withQueryFields(array $fields): RequestOptions
    {
        // Do some validation here
        $new = clone $this;
        $new->ignoreQueryFields = $fields;

        return $new;
    }

    // Add rest of methods + getters
}

Then add a setRequestOptions method to set it, that only accepts the RequestOptions class.
The same needs to be done for the response options.

This has 2 main advantages:
A) It is immediately clear for the user what options can be set trough the class and autocomplete
B) Any value is validated in the object, so you can straight up use the values of the options in the rest of the code.

If you want to have a compatibility layer for legacy, you can add a named constructor in the RequestOptions to build it fromArray, but imho that is just a nice to have.

@allejo

This comment has been minimized.

Copy link
Owner Author

@allejo allejo commented Jan 10, 2019

I really like the fact that that approach would allow IDEs to automatically find appropriate methods; documenting complex arrays is something I wish phpdoc would support. Validation of configuration options is also something I really like.

So I'm imagining the usage of this method to look something like this:

$rq = new RequestOptions();
$rq
  ->ignoreHostname()
  ->ignoreQueryFields(['one', 'two'])
;

$rs = new ResponseOptions();
$rs
  ->ignoreHeaders(['x-api-key'])
  ->setBodyScrubbers([...])
;

VCRCleaner::setRequestOptions($rq);
VCRCleaner::setResponseOptions($rs);

While I really love the benefits that this approach would provide, I'm not sure how I feel about introducing an extra layer of abstraction just to configure the cleaner.

I feel like the cleaner is something you quickly configure once and then forget about it beause it should just work in the background. Or at least that's been my only use case so far, I'm not sure how anyone else is using it so I'd appreciate your thoughts.

@janvernieuwe

This comment has been minimized.

Copy link
Contributor

@janvernieuwe janvernieuwe commented Jan 10, 2019

Of course I don't change it often, but I need to create new ones often enough when i use phpvcr in combination with authenticated API's (micro service that consumes an API, I have lots of them).

It is not a deal breaker for me, but it is nice for anyone who wants to use the lib in the future.
Plus it's not like this extra layer is very complex and you don't need to do array merging with defaults with this.

(ps: you need to assign it to a var if immutable setters are used ;) )

@pfuhrmann

This comment has been minimized.

Copy link
Contributor

@pfuhrmann pfuhrmann commented Jan 10, 2019

I prefer to have a simple array also, for now. There are only a few configuration options which are well documented. As @allejo said, those are like: configure once and leave it.

With any project, you would usually have JSON file or YAML file for config and you always have to look up configuration values. I do believe this allows for more flexibility and less refactoring when modifying configuration options.

@allejo

This comment has been minimized.

Copy link
Owner Author

@allejo allejo commented Jan 15, 2019

I think I'm going to leave an array be the main way to configure this project. A builder can be added to the project that can then be exported as an array so it'll remain compatible.

@pfuhrmann

This comment has been minimized.

Copy link
Contributor

@pfuhrmann pfuhrmann commented Jan 16, 2019

@allejo can you please create 1.0.0 release now that #5 was merged? Excited to use new features 馃憤

@allejo

This comment has been minimized.

Copy link
Owner Author

@allejo allejo commented Jan 16, 2019

v1.0.0 has been tagged 馃帀 Thank you for all the feedback, it's really appreciated!

@allejo allejo closed this Jan 16, 2019
@allejo allejo unpinned this issue Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.