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.

Sign up for GitHub

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

Jump to bottom

Some maintenance work #77

Merged
merged 7 commits into from
Nov 20, 2022
Merged

Some maintenance work #77

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
89f27e5
Allow org_heigl/hyphenator in ^3.0
lyrixx May 30, 2022
4a456a7
Simplify composer.json 'scripts' section
lyrixx May 30, 2022
aea4cac
Drop support for Symfony < 4.4 + Add support for ^6.0
lyrixx May 30, 2022
a3269c8
Run tests on PHP 8.1
lyrixx May 30, 2022
4b458ba
Fix encoding issues
HedicGuibert Jun 4, 2022
5eaf040
disable http_method_override
HedicGuibert Jun 4, 2022
8c00c9d
Use NULL as default encoding if no detected match + CS
damienalexandre Nov 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/cicd.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ jobs:
php-versions:
- '7.4'
- '8.0'
- '8.1'

steps:
- uses: actions/checkout@v2
Expand All @@ -43,6 +44,7 @@ jobs:
php-versions:
- '7.4'
- '8.0'
- '8.1'

steps:
- uses: actions/checkout@v2
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ CHANGELOG

### 1.4.0 (not released yet) ###

* Drop support for Symfony < 4.4
* Add support for Symfony ^6.0
* Run tests on PHP 8.1

### 1.3.0 (2022-05-30) ###

* Add PHAR generation to use the library in CLI context
Expand Down
12 changes: 6 additions & 6 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@
"php": ">=7.4",
"ext-mbstring": "*",
"lib-libxml": "*",
"org_heigl/hyphenator": "~2.6.0"
"org_heigl/hyphenator": "^2.6 || ^3.0"
},
"require-dev": {
"friendsofphp/php-cs-fixer": "^3.3.2",
"symfony/phpunit-bridge": "^5.0",
"symfony/framework-bundle": "^3.4.26|^4.1.12|^5.0",
"symfony/twig-bundle": "^3.4.26|^4.1.12|^5.0",
"symfony/yaml": "^3.4.26|^4.1.12|^5.0"
"symfony/phpunit-bridge": "^5.4.8 || ^6.0",
"symfony/framework-bundle": "^4.4 || ^5.0 || ^6.0",
"symfony/twig-bundle": "^4.4 || ^5.0 || ^6.0",
"symfony/yaml": "^4.4 || ^5.0 || ^6.0"
},
"conflict": {
"ext-apc": "3.1.11"
Expand All @@ -35,7 +35,7 @@
"psr-4": { "JoliTypo\\Tests\\": "tests/JoliTypo/Tests" }
},
"scripts": {
"test": "vendor/bin/simple-phpunit -c phpunit.xml.dist",
"test": "vendor/bin/simple-phpunit",
"cs": "vendor/bin/php-cs-fixer fix"
}
}
21 changes: 14 additions & 7 deletions src/JoliTypo/Fixer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,16 +305,13 @@ private function loadDOMDocument($content)
$dom->substituteEntities = false;
$dom->formatOutput = false;

// Change mb and libxml config
// Change libxml config
$libxmlCurrent = libxml_use_internal_errors(true);
$mbDetectCurrent = mb_detect_order();
mb_detect_order('ASCII,UTF-8,ISO-8859-1,windows-1252,iso-8859-15');

$loaded = $dom->loadHTML($this->fixContentEncoding($content));

// Restore mb and libxml config
// Restore libxml config
libxml_use_internal_errors($libxmlCurrent);
mb_detect_order(implode(',', $mbDetectCurrent));

if (!$loaded) {
throw new InvalidMarkupException("Can't load the given HTML via DomDocument");
Expand Down Expand Up @@ -345,7 +342,15 @@ private function fixContentEncoding($content)
$content = $hack . $content;
}

$encoding = mb_detect_encoding($content);
$encoding = '';

foreach (['UTF-8', 'ASCII', 'ISO-8859-1', 'windows-1252', 'iso-8859-15'] as $testedEncoding) {
if (mb_detect_encoding($content, $testedEncoding, true)) {
$encoding = $testedEncoding;
break;
}
}

$headPos = mb_strpos($content, '<head>');

// Add a meta to the <head> section
Expand All @@ -356,7 +361,9 @@ private function fixContentEncoding($content)
mb_substr($content, $headPos);
}

$content = mb_convert_encoding($content, 'HTML-ENTITIES', $encoding);
if ('UTF-8' !== $encoding) {
$content = mb_convert_encoding($content, 'UTF-8', $encoding);
Copy link
Member

Choose a reason for hiding this comment

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

Actually i'm not sure about this

Copy link
Member

Choose a reason for hiding this comment

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

Yes it should be HTML-ENTITIES

Copy link
Member

Choose a reason for hiding this comment

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

And there is no need to test if it's UTF-8.

Copy link
Member

Choose a reason for hiding this comment

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

Using mb_convert_encoding to convert to HTML entities is deprecated in PHP 8.2 and did not function well previously : php/php-src@9308974

And we don't want to ue html_entity_decode because it will break the fixer if the user pass something like 1 > 3 or <3.

My concern was more about the fact that we set the charset to $encoding but then we encode the content to UTF-8. This is weird. Either we don't set the charset or we don't convert IMO.

}
}

return $content;
Expand Down
1 change: 1 addition & 0 deletions tests/JoliTypo/Tests/Bridge/app/config/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ framework:
resource: "%kernel.project_dir%/tests/JoliTypo/Tests/Bridge/app/config/routing.yml"
strict_requirements: ~
secret: "ThisTokenIsEnoughSecretForOurTests"
http_method_override: false
twig:
strict_variables: '%kernel.debug%'

Expand Down
2 changes: 1 addition & 1 deletion tests/JoliTypo/Tests/Html5Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public function testFullPageMarkup()
HTML;

$fixed = <<<'STRING'
&#8220;Who Let the Dogs Out?&#8221; is a song written and originally recorded by Anslem Douglas (titled &#8220;Doggie&#8221;).
“Who Let the Dogs Out?” is a song written and originally recorded by Anslem Douglas (titled “Doggie”).
Copy link
Member

Choose a reason for hiding this comment

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

Why does the behaviour changed here?! I don't get it :/

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, encoding seems to have had numerous changes in PHP 8.1.
However, this is what we expected to get in similar tests :
https://github.com/jolicode/JoliTypo/blob/master/tests/JoliTypo/Tests/Fixer/EnglishQuotesTest.php#L42-L45
https://github.com/jolicode/JoliTypo/blob/master/tests/JoliTypo/Tests/JoliTypoTest.php#L175

Copy link
Member Author

Choose a reason for hiding this comment

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

The new version look good to me 👍🏼

STRING;

$this->assertEquals($fixed, $fixer->fix($html));
Expand Down