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

Require $this->headers to be instance of Headers #124

Closed
wants to merge 2 commits into from

Conversation

paragonie-scott
Copy link
Contributor

@paragonie-scott paragonie-scott commented Nov 29, 2016

Turns a fatal error into a catchable Exception (which allows users to have a helpful stack trace).

This sort of change will be unnecessary in PHP 7 (hooray return type declarations and catchable TypeErrors).

@paragonie-scott
Copy link
Contributor Author

There may be other areas where it would be beneficial to convert fatal errors to catchable exceptions besides the instance I found here.

@Ocramius
Copy link
Member

There may be other areas where it would be beneficial to convert fatal errors to catchable exceptions besides the instance I found here.

Wouldn't bother too much about it, given the upcoming drop of PHP 5.x support

@Ocramius Ocramius added the bug label Dec 17, 2016
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Tests for the introduced fixes are missing

@@ -294,10 +294,16 @@ public function getHeaders()
*/
public function getHeader($name, $format = null)
{
$header = $this->getHeaders()->get($name);
$headers = $this->getHeaders();
Copy link
Member

Choose a reason for hiding this comment

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

The API of getHeaders specifies that a Headers instance MUST be returned. If that is not true, then getHeaders needs fixing, and you are fixing a symptom here.

@@ -383,7 +389,13 @@ public function __get($name)
*/
public function __isset($name)
{
return $this->getHeaders()->has($name);
$headers = $this->getHeaders();
Copy link
Member

Choose a reason for hiding this comment

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

The API of getHeaders specifies that a Headers instance MUST be returned. If that is not true, then getHeaders needs fixing, and you are fixing a symptom here.

@paragonie-scott
Copy link
Contributor Author

Right, I'll send a superseding PR.

@paragonie-scott paragonie-scott deleted the patch-1 branch May 5, 2017 06:10
paragonie-scott added a commit to paragonie-scott/zend-mail that referenced this pull request May 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants