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

Lazy load structured Headers #250

Merged
merged 1 commit into from
Oct 23, 2017
Merged

Conversation

Slamdunk
Copy link
Collaborator

@Slamdunk Slamdunk commented Oct 19, 2017

Following @wujku issue #246 here I'm proposing a change to Message\Headers to resolve it.

Since the very first tag in 2013, header validation and specifically EmailAddresses and Date header validation are done at Message\Headers construction, meaning that if any error occurs in any header, no header at all can be queried.

An invalid Date would make this code raising the Exception:

$message = $mailbox->getMessage(1);
// throws InvalidDateHeaderException
var_dump($message->getSubject());

While I would expect it to be thrown only when needed:

$message = $mailbox->getMessage(1);
// Ok
var_dump($message->getSubject());
// throws InvalidDateHeaderException
var_dump($message->getDate());

This PR leaves the decode in the Header construction, so all headers will always be UTF-8, but moves the logic to the Message\AbstractMessage API.

Is this a BC break?

Yes and No.

Yes because this code would behave differently:

// Message\Headers is an ArrayIterator
$headers = $message->getHeaders();
var_dump($headers['to']);
// Before: array of EmailAddress
// After: array of \stdClass
var_dump($headers['date']);
// Before: class DateTimeImmutable
// After: string "Wed, 27 Sep 2017 12:48:51 +0200"

No because the previous example is not a public API usage (i.e. interfaces signatures), so it wouldn't be an incompatible API change. In fact the following code would behave the same:

var_dump($message->getTo());
// Before & After: array of EmailAddress
var_dump($message->getDate());
// Before & After: class DateTimeImmutable

We could in theory make the change fully backward compatible, but we would need to overwrite a lot of ArrayIterator functions: current, getArrayCopy, next, offsetGet, get, seek, serialize etc. Too complex, doesn't worth the effort.

Moreover, I like the change to the Message\Headers contents because the user would be able to choose to query the raw header through the Message\Headers instance, or the parsed header through the Message API.

At the time of writing, I'm tagging this to the 1.1 milestone expected to be released on 2017-11-06.
If we decide this to be a complete BC break, I'll move it to the 2.0 milestone, expected to be released on 2018-03-05 (at least 6 months after the previous major release to gather other major suggestions).

@ddeboer @wujku what do you think?

@Slamdunk Slamdunk added this to the 1.1 milestone Oct 19, 2017
@Slamdunk Slamdunk requested a review from ddeboer October 19, 2017 09:34
@Slamdunk Slamdunk changed the title Lazy load Headers Lazy load structured Headers Oct 19, 2017
@ddeboer ddeboer merged commit 6534a76 into ddeboer:master Oct 23, 2017
Copy link
Owner

@ddeboer ddeboer left a comment

Choose a reason for hiding this comment

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

I agree with both your reasons both for implementing this change and with your statement that it’s reasonably BC.

@Slamdunk Slamdunk deleted the lazy_load_headers branch October 23, 2017 14:12
@Slamdunk Slamdunk removed the BC break label Oct 23, 2017
@Slamdunk
Copy link
Collaborator Author

@ddeboer In the release I'll write a clear note about this change, hoping not to break anyone code.

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

Successfully merging this pull request may close these issues.

2 participants