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

Add replace argument to IntegrationTestTrait::configRequest #17587

Open
cnizzardini opened this issue Feb 18, 2024 · 9 comments
Open

Add replace argument to IntegrationTestTrait::configRequest #17587

cnizzardini opened this issue Feb 18, 2024 · 9 comments

Comments

@cnizzardini
Copy link
Contributor

cnizzardini commented Feb 18, 2024

Description

If I have a common configured request in my setUp method such as

    public function setUp(): void
    {
        parent::setUp();
        $this->requestAsJson();
        $this->configRequest([
            'headers' => [
                'Authorization' => 'Bearer ' . (new JwtHelper())->withSubscriptions([SubscriptionPlanEnum::STARTER]),
            ]
        ]);

And need to replace it only in a few instances such as:

    public function test_index_with_growth_plan(): void
    {
        $this->configRequest([
            'headers' => [
                'Authorization' => 'Bearer ' . (new JwtHelper())->withSubscriptions([SubscriptionPlanEnum::GROWTH]),
            ]
        ]);

This adds two bearer tokens because configRequest() uses array_merge_recursive(). Adding a $replace bool argument that defaults to false but uses array_replace_recursive() if true would be a nice option here rather than having to manually clear the $this->_request property in these cases.

Unless there is another way to accomplish this... and yes I understand this is next level laziness, but this appears to be how it worked in CakePHP 4.

CakePHP Version

5.0

@markstory
Copy link
Member

This adds two bearer tokens because configRequest() uses array_merge_recursive(). Adding a $replace bool argument that defaults to false but uses array_replace_recursive() if true would be a nice option here rather than having to manually clear the $this->_request property in these cases.This adds two bearer tokens because configRequest() uses array_merge_recursive(). Adding a $replace bool argument that defaults to false but uses array_replace_recursive() if true would be a nice option here rather than having to manually clear the $this->_request property in these cases.

What if we had a $this->resetRequest(); method to reset the request? I think that would be clearer to read, and not much harder to write especially with autocomplete.

@markstory markstory added this to the 5.1.0 milestone Feb 19, 2024
@cnizzardini
Copy link
Contributor Author

This amounts to a wrapper around $this->_request = []. It is cleaner than working directly with an underscored property, but still requires rebuilding the entire request. I don't want to harp on this too much as this is a minor gripe. I leave this call to you.

@markstory
Copy link
Member

It is cleaner than working directly with an underscored property, but still requires rebuilding the entire request.

That's fair. Would having a method that does the replacement work instead? Perhaps replaceRequest? 🤷

@cnizzardini
Copy link
Contributor Author

replaceRequest is probably the cleanest approach of all rather than mangling an existing method.

@cnizzardini
Copy link
Contributor Author

Would you say this method should clear the request and then add? Or should it do an array_replace_recursive?

@markstory
Copy link
Member

Would you say this method should clear the request and then add? Or should it do an array_replace_recursive?

A recursive replace is likely the most ergonomic solution. It emulates 'merging' without the tedious duplicate values that merging arrays can create.

@cnizzardini
Copy link
Contributor Author

Is there a specific reason this is targeted as a 5.1 minor feature. Does Cake strictly put features into minors and fixes into revisions? Only curious.

@cnizzardini
Copy link
Contributor Author

cnizzardini commented Mar 7, 2024

Also I briefly looked at doing this, the solution is obvious, the question is what kind of tests would be involved here. There doesn't seem to be tests specifically for configRequest, despite it clearly being covered via its use within tests.

@markstory
Copy link
Member

There doesn't seem to be tests specifically for configRequest, despite it clearly being covered via its use within tests.

I'm ok with the implicit testing. If the sample controller method would not generate the correct response if the tokens were present then we have a good integration test that can be documented as to what it covers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants