Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Component][DomCrawler] - MIME Type Handling for charset only #7593

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

egulias commented Apr 7, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #7491
License MIT
Doc PR -

@fabpot fabpot commented on an outdated diff Apr 8, 2013

src/Symfony/Component/DomCrawler/Crawler.php
@@ -89,6 +89,10 @@ public function addContent($content, $type = null)
$type = 'text/html';
}
+ if (false !== strpos($type, 'charset=')) {
@fabpot

fabpot Apr 8, 2013

Owner

I would have dealt with this in the same condition below (line 102) and set the type there when pos is 0.

Contributor

egulias commented Apr 8, 2013

After giving a thought on your comment I found that if "text/html" is not placed first, the control would (in either line) be changing the $type always to "text/html", I added a test case for that case ("charset=UTF-8; text/html"). Moved the preg_match replace after the charset control since the $type could still be guessed.

@fabpot fabpot commented on the diff Apr 9, 2013

src/Symfony/Component/DomCrawler/Crawler.php
$charset = 'ISO-8859-1';
if (false !== $pos = strpos($type, 'charset=')) {
$charset = substr($type, $pos + 8);
if (false !== $pos = strpos($charset, ';')) {
$charset = substr($charset, 0, $pos);
}
+
+ if (strlen($charset) === strlen($type) - 8) {
@fabpot

fabpot Apr 9, 2013

Owner

isn't it equivalent to 0 === $pos?

@fabpot

fabpot Apr 9, 2013

Owner

The first $pos variable, not the second one.

@egulias

egulias Apr 9, 2013

Contributor

I think not because 0 === $pos is that "charset=" is at the beginning of $type, not that there is nothing else after it ( this is the assert).

Owner

fabpot commented Apr 11, 2013

I'm closing this PR as a won't fix as per the RFC, this is just not valid.

@fabpot fabpot closed this Apr 11, 2013

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