【Unsolicited PR】I changed the download method to testable.

[ytetsuro]

I think Response::download method is not testable.
because use exit and echo.
my solution is following.

1. create Download Response class.
2. to exit when Download Response class returns from Controller method.

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• Conforms to style guide

[lonnieezell]

By and large, I'm ok with this change. However, you're duplicating a lot of code by extending Message and not Response. That's a maintenance issue. Please extend the Response class, and remove any duplicate methods that don't have a reason for being overridden.

[lonnieezell]

Need to update the exception style. This would probably be best added to FrameworkException and called like:

throw FrameworkException::forInvalidFilePath(lang(''));

Also - notice the error should be localized, please.

[ytetsuro]

Thanks.
fixed.

[lonnieezell]

Is there a reason this method overriding the base method?

[lonnieezell]

Wouldn't it be simpler to extend Response and not duplicate a number of the methods here, especially with slightly different implementations? Seems like a maintenance issue.

[lonnieezell]

@lonnieezell

Thank you for reviewing
I am very very very happy.😻

Certainly I understand it well.
However, when I inherited Response class I felt the following discomfort.

I think in the CodeIgniter class that finally receives the DownloadResponse class, there are many conditional branches that depend on the Response class, and if i inherit the Response class, maintenance will be difficult.
DownloadResponse will have redirect and download methods when extends Response.
I think should not have redirect and response methods in DownloadResponse.

Or should we inherit Response thinking that these are not much problems?

That's a good point. I looked at the flow there, and you're right it would be simpler to extend Message directly for this. In that case - I can't imagine all of those duplicate methods are actually used to force a download? Please take a look and see how much you can get rid of from the original Response class.

[ytetsuro]

Please take a look and see how much you can get rid of from the original Response class.

Certainly, by inheriting the Respone class you can erase many methods from the DownloadResponse class.
But, i analyzed Response class methods.
The analysis results are as follows.

Reusable methods.

• pretend
• getStatusCode
• setStatusCode
• getReason
• setDate
• setLastModified
• setContentType

Method that needs to be changed so that it can not be called.

• setJSON
• getJSON
• setXML
• getXML
• setCache
• getBody
• redirect
• setCookie
• hasCookie
• getCookie
• deleteCookie
• sendCookies
• download

Methods that need to be overwritten.

• __construct
• noCache
• send
• sendHeaders
• sendBody

Overwrite is unnecessary because it is closed but DownloadResponse does not need unnecessary methods.

• formatBody

I feel that there are many methods that need to be changed so that they can not be called.
Is it better to inherit the Response class really better?

[ytetsuro]

@lonnieezell

Excuse me.
It is possible that I could not understand what you are saying.

Is that what you are saying does not need to implement RequestInterface?
If so, I'm sorry.
That is definitely the right decision.

[lonnieezell]

@ytetsuro

Is that what you are saying does not need to implement RequestInterface?

I assume you mean ResponseInterface not the RequestInterface?

If so - we still need to implement ResponseInterface - however the interface only requires 9 methods.

[ytetsuro]

@lonnieezell

I assume you mean ResponseInterface not the RequestInterface?

Sorry...
This is typo.
ResponseInterface is my intention.

If so - we still need to implement ResponseInterface - however the interface only requires 9 methods.

Why do you think need to implements the ResponseInterface?

[lonnieezell]

Why do you think need to implements the ResponseInterface?

Simply because it is a Response and should be cast as such. While we may or may not do any checks on that currently, we might in the future and no sense not following the interface now. Besides, this should still go through the after filters (which I just realized I failed to check in your implementation) before being sent, which requires objects implementing ResponseInterface to work.

[ytetsuro]

@lonnieezell

Simply because it is a Response and should be cast as such. While we may or may not do any checks on that currently, we might in the future and no sense not following the interface now. Besides, this should still go through the after filters (which I just realized I failed to check in your implementation) before being sent, which requires objects implementing ResponseInterface to work.

I understand.
I correct it so that it go through the after filters in CodeIgniter class.

but, add 13 unnecessary methods when extend the Response class.
I think that it would be better to extend the Message class even though the maintenance problem is considered, but what do you think?

[lonnieezell]

but, add 13 unnecessary methods when extend the Response class.

All I said is that it should implement the ResponseInterface. It's not necessary to extend the Response class itself. Extending Message is fine, as long as you implement the ResponseInterface interface. That interface only has 9 methods specified, I think. A few might be unnecessary, in which case maybe throwing an exception so the developer can correct his code?

[ytetsuro]

I think. A few might be unnecessary, in which case maybe throwing an exception so the developer can correct his code?

Sure.
Modify it to throwing an exception.

[ytetsuro]

@lonnieezell

All have been corrected.
Would you take a moment to review again.

[ytetsuro]

Merged very very very thanks.❤️