Skip to content

Add integration for VCR plugin #333

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 29, 2019
Merged

Add integration for VCR plugin #333

merged 7 commits into from
May 29, 2019

Conversation

GaryPEGEOT
Copy link
Contributor

@GaryPEGEOT GaryPEGEOT commented Apr 15, 2019 •
edited
Loading

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes #330
Documentation php-http/documentation#267
License MIT

What's in this PR?

Add an easy way to configure the VCR plugin

Example Usage

# config/packages/dev/httplug.yaml
httplug:
    # ...
    clients:
        # ...
        acme:
            # ...
            plugins:
                -
                    vcr:
                        mode: replay # record | replay | replay_or_record
                        recorder: filesystem # optional
                        fixtures_directory: '%kernel.project_dir%/fixtures/http' # mandatory for "filesystem" recorder
                        naming_strategy: service_id.of.naming_strategy # optional
                        naming_strategy_options: # optional
                            hash_headers: []
                            hash_body_methods: []

Checklist

dbu
dbu reviewed Apr 16, 2019
View reviewed changes
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks! this looks good. so there can be a different VCR for each client? that makes sense to me.

i have some suggestions for better readability and some requests to keep the code style consistent

@GaryPEGEOT
Copy link
Contributor Author

@dbu about the travis failure, should I try to lower VCR plugin requirements to Symfony 2.8?

dbu
dbu reviewed Apr 17, 2019
View reviewed changes
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks. only a few points left.

about the php version: argh! i think we should create a new minor version of this bundle that is only php 7.1 and newer. @php-http/owners would you agree with that or do you have objections?

src/DependencyInjection/Configuration.php Outdated
->enumNode('mode')
->info('What should be the behavior of the plugin?')
->values(['record', 'replay', 'replay_or_record'])
->defaultValue('replay_or_record')
Copy link
Collaborator

Choose a reason for hiding this comment

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

the defensive value would be to default to "replay". i am inclined to make this have no default and be a required field to force people to actively decice which mode is right for them (if they enable the vcr at all)

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 removed the default value

dbu
dbu approved these changes May 20, 2019
View reviewed changes
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

awesome! i only have a couple of nitpicks, otherwise this looks great!

can you do the documentation pull request to add a section next to "Authentication" in http://docs.php-http.org/en/latest/integrations/symfony-bundle.html#plugins and to add to http://docs.php-http.org/en/latest/integrations/symfony-full-configuration.html (please mention that the vcr plugin is a separate package that needs to be required with composer)

tests/Unit/DependencyInjection/HttplugExtensionTest.php Outdated
$this->assertContainerBuilderHasService('httplug.plugin.vcr.recorder.in_memory', InMemoryRecorder::class);

foreach ($services as $service) {
$this->assertContainerBuilderHasService("$prefix.$service");
Copy link
Collaborator

Choose a reason for hiding this comment

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

we do not use the inline string expansion thing. can you please rewrite this as $prefix.'.'.$service? (its a bit weird, but i'd like to keep a consistent code style)

src/DependencyInjection/HttplugExtension.php Outdated
*
* @return ChildDefinition
*/
private function getChildDefinition($parent)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we name this createChildDefinition? i was trying to figure out why we can access a definition before the service file defining the vcr definitions has been loaded... until i read this method and realized it is a factory method (for the BC thingy).

src/DependencyInjection/HttplugExtension.php Outdated
/**
* @param string $parent the parent service id
*
* @return ChildDefinition
Copy link
Collaborator

Choose a reason for hiding this comment

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

|DefinitionDecorator

can you please add a description? "BC for old Symfony versions. Remove this method and use new ChildDefinition directly when we drop support for Symfony 2."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! I also added the doc PR.

@dbu
Copy link
Collaborator

dbu commented May 20, 2019

oh, and the build fails where we use php < 7.1 because vcr is only available for recent php versions. i think its time to raise the php version for the bundle. (afaik we said we can allow outdated versions until it becomes a problem - now it did become a problem)

@GaryPEGEOT GaryPEGEOT mentioned this pull request May 20, 2019
Closed
dbu
dbu approved these changes May 24, 2019
View reviewed changes
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks, looks good to me!

the build needs some love but as far as i can tell, this PR is not adding to the problems.

@Nyholm are you ok merging as is?

@dbu dbu merged commit bad3276 into php-http:master May 29, 2019
@dbu
Copy link
Collaborator

dbu commented May 29, 2019

thanks a lot for this contribution! and thanks for your patience with all my nitpicks ;-)

@GaryPEGEOT GaryPEGEOT deleted the feature/vcr-plugin-integration branch May 29, 2019 10:13
@localheinz localheinz mentioned this pull request May 30, 2019
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbu dbu dbu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Plugin] Add configuration for VCR plugin
2 participants
@GaryPEGEOT @dbu