cakephp icon indicating copy to clipboard operation
cakephp copied to clipboard

Add replace argument to IntegrationTestTrait::configRequest

Open cnizzardini opened this issue 2 years ago • 9 comments

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

cnizzardini avatar Feb 18 '24 13:02 cnizzardini

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 avatar Feb 19 '24 03:02 markstory

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.

cnizzardini avatar Feb 19 '24 20:02 cnizzardini

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? 🤷

markstory avatar Feb 20 '24 16:02 markstory

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

cnizzardini avatar Feb 20 '24 17:02 cnizzardini

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

cnizzardini avatar Feb 20 '24 17:02 cnizzardini

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.

markstory avatar Feb 23 '24 03:02 markstory

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 avatar Mar 07 '24 02:03 cnizzardini

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.

cnizzardini avatar Mar 07 '24 02:03 cnizzardini

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.

markstory avatar Mar 13 '24 03:03 markstory

It it possible to ask why it was changed to a array_merge_recursive? Does it help in some tests ? If yes how ?

I have a lot of tests where I have simply overwritten the request with this->configRequest() in CakePhp4 and I don't understand the benefit of the change ?

mcube27 avatar Jul 08 '24 14:07 mcube27

It was changed in this PR https://github.com/cakephp/cakephp/pull/16942 to actually allow merging data when calling $this->configRequest() multiple times (which was needed to actually allow requestAsJson() to work)

If you desire to reset the request config you can easily call $this->_request = [] yourself as well.

LordSimal avatar Jul 08 '24 14:07 LordSimal