Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Deprecated multiple behavior methods #158

Conversation

thomasvargiu
Copy link
Contributor

@thomasvargiu thomasvargiu commented Oct 17, 2018

In order to improve code quality and easily add type hints in a future release, I started to deprecate some methods with multiple behavior and different return types based on arguments.

  • Zend\Http\Request::getHeader() is deprecated
  • Zend\Http\Request::getHeaders() with arguments is deprecated
  • Zend\Http\Request::getQuery() with arguments is deprecated
  • Zend\Http\Request::getPost() with arguments is deprecated
  • Zend\Http\Request::getFiles() with arguments is deprecated

The deprecation is for methods used as a "proxy".
Clients can access to them getting the collection first.

$request->getHeader('header-name'); // deprecated
$request->getHeaders('header-name'); // deprecated

$request->getHeaders()->get('header-name'); // currently suggested access
$request->getQuery('name'); // deprecated

$request->getQuery()->get('name'); // currently suggested access
$request->getPost('name'); // deprecated

$request->getPost()->get('name'); // currently suggested access
$request->getFiles('name'); // deprecated

$request->getFiles()->get('name'); // currently suggested access

This will allow to introduce type hints for these methods in the future , something like:

use Zend\Stdlib\ParametersInterface;
use Zend\Http\Headers;

class Request
{
    public function getHeaders(): Headers;
    public function getQuery(): ParametersInterface;
    public function getPost(): ParametersInterface;
    public function getFiles(): ParametersInterface;
}
``` 

@froschdesign
Copy link
Member

@thomasvargiu
I'm sorry, but in my opinion this pull request is unacceptable.

I see the following problems:

  • an explanation is missing why this functionality should be removed
  • the error messages do not contain an alternative way to get the values
  • the documentation is not updated

And where is the plan or the pull requests to update components like zend-mvc?

@thomasvargiu
Copy link
Contributor Author

thomasvargiu commented Oct 17, 2018

@froschdesign I was working on #157. In order to improve the code in a major release is useless to call

$request->getHeader('name');

when you can access the entire collection and get it with:

$request->getHeaders()->get('name');

IMHO, having a different behavior and a different return values based on arguments is not a good code.
Introducing this deprecation will help us to use strict types, reducing tests and making a better code.

Of course we can improve deprecation messages.

It is a deprecation, but we can remove that behavior only in major release.

@froschdesign
Copy link
Member

froschdesign commented Oct 17, 2018

@thomasvargiu
I know the usage, but that's not the topic. You open a PR without any explanation, but with changes which generates warning messages.
Please always add a full explanation in the title and / or description. That will help you and everyone else. Thanks!


The question also remains:

And where is the plan or the pull requests to update components like zend-mvc?

@thomasvargiu
Copy link
Contributor Author

thomasvargiu commented Oct 17, 2018

zend-http is still on version 2.x. zend-mvc currently requires zend-http:^2.x. We can work to improve zend-http and release a 3.x version (maybe with type hints?). As usual, other libraries should be compliant with the supported dependencies.

Changes are easy because the same behavior exists in zend-http:^2.x and will exist in a future major release.

I'm going to improve the PR description. DONE

@weierophinney
Copy link
Member

zend-http is still on version 2.x. zend-mvc currently requires zend-http:^2.x. We can work to improve zend-http and release a 3.x version (maybe with type hints?).

zend-mvc v4 will be more likely to use PSR-7 (see thezend-mvc roadmap document — if we even decide to work towards a v4 at this point), which leaves the future of zend-http pretty much in maintenance mode. I do not see any reason at this point to do deprecations when we are unlikely to release a new major version ever.

As such, I'm going to close this.

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

Successfully merging this pull request may close these issues.

3 participants