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

Content-Transfer-Encoding value not case sensitive #5598

Merged
merged 1 commit into from Jan 2, 2014
Merged

Content-Transfer-Encoding value not case sensitive #5598

merged 1 commit into from Jan 2, 2014

Conversation

denis-sokolov
Copy link
Contributor

No description provided.

@denis-sokolov
Copy link
Contributor Author

Travis found trailing spaces

Oh, so proposing pull requests from a Github web-editor is not such a great idea.
Will push a fix right away.

@denis-sokolov
Copy link
Contributor Author

The latest test failure is unrelated to the pull request.

@samsonasik
Copy link
Contributor

you should provide unit test

@denis-sokolov
Copy link
Contributor Author

Unit test added.

@macnibblet
Copy link
Contributor

According to your comment the RFC says it's not case sensitive.... That would mean your call to strtolower is pointless ?

@denis-sokolov
Copy link
Contributor Author

According to RFC, anyone can call setTransferEncoding with quOtED-PrinTablE and that is the same as if when they call it with quoted-printable.
Pre-pr code throws an exception on the following line. Post-pr code handles it correctly.

Similarly, the output of getFieldValue can be quoted-printable or QUoTeD-PrintablE.
Current implementation always returns the lowercase variant,
but in my unit test, I do not want to rely on this implementation detail, that is why I call strtolower.

Does that sound a bit clearer, @macnibblet?

@Maks3w
Copy link
Member

Maks3w commented Dec 15, 2013

@denis-sokolov Could you apply the same fix to the equivalent HTTP Header?

@denis-sokolov
Copy link
Contributor Author

@Maks3w, do you mean the Http\-namespaced class?
That one does not have the problem, because it did not even try to implement anything.
Quoting:

// @todo implementation details
$header->value = $value;

Why keep two similar classes, though?
If needed for semantic purposes, can't you make Http\ one use Mail\ one internally?
Or vice-versa. This way improvements to one would automatically work with the other.

@Maks3w
Copy link
Member

Maks3w commented Dec 15, 2013

Each header is proposed from a different organization and with different porpouse. So different implementations are expected.

@Maks3w
Copy link
Member

Maks3w commented Dec 15, 2013

Just normalize the value in the other class.

@denis-sokolov
Copy link
Contributor Author

What do you want me to normalize?
Http\ class conforms to the spec already.

@denis-sokolov
Copy link
Contributor Author

Besides, if the classes are meant to be so different, with different purposes and different implementations,
I suggest these placing these improvements to Http\ class in a different pull request by a different author. :)

@ghost ghost assigned Maks3w Jan 2, 2014
Maks3w added a commit that referenced this pull request Jan 2, 2014
Conflicts:
	library/Zend/Mail/Header/ContentTransferEncoding.php
Maks3w added a commit that referenced this pull request Jan 2, 2014
@Maks3w Maks3w merged commit 585c1de into zendframework:master Jan 2, 2014
@denis-sokolov denis-sokolov deleted the patch-1 branch January 2, 2014 23:25
weierophinney pushed a commit to zendframework/zend-mail that referenced this pull request May 14, 2015
…il-content-transfer-enconding-non-sensitive'
weierophinney pushed a commit to zendframework/zend-mail that referenced this pull request May 14, 2015
Conflicts:
	library/Zend/Mail/Header/ContentTransferEncoding.php
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.

None yet

4 participants