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

UTF-8 with BOM #461

Closed
fl3pp opened this issue Nov 7, 2018 · 3 comments · Fixed by #515
Closed

UTF-8 with BOM #461

fl3pp opened this issue Nov 7, 2018 · 3 comments · Fixed by #515

Comments

@fl3pp
Copy link

fl3pp commented Nov 7, 2018

Hello together,

Unfortunately Pico didn't render my YAML content correctly. I was searching several hours until I found the issue in the regex:

$markdown = preg_replace($metaHeaderPattern, '', $rawContent, 1);

I was able to reproduce the problem in a small script:

$metaHeaderPattern = "/^(\/(\*)|---)[[:blank:]]*(?:\r)?\n"
. "(?:(.*?)(?:\r)?\n)?(?(2)\*\/|---)[[:blank:]]*(?:(?:\r)?\n|$)/s";

// returns false
$rawContent = file_get_contents("C:\FILE WITH BOM");
$isMatch = preg_match($metaHeaderPattern, $rawContent) == 1;

// return true
$rawContent = file_get_contents("C:\FILE WITHOUT BOM");
$isMatch = preg_match($metaHeaderPattern, $rawContent) == 1;

It seems that the regex replacement (and the markdown parsing too) is not able to handle files with UTF-8 BOM headers correctly.
One should not add BOMs anyway, but many text-editor do without users knowing it.

Could you add support for BOMs?

@PhrozenByte
Copy link
Collaborator

Not sure about whether we should add explicit support for UTF-8 BOM or not. UTF-8 BOM are known to cause problems, so I'm not sure whether we should "promote" them by explicitly supporting them...

@ALL, feedback is appreciated!

@fl3pp
Copy link
Author

fl3pp commented Nov 8, 2018

I understand what you mean and agree with you that this concern is worth thinking about before implementing.

But in my opinion a corresponding error message should be shown, explicitely dissallowing BOMs because they are not supported. Or at least mention this topic in the documentation.

Otherwise users (like me) are being confronted with a really confusing behavior.

@fl3pp
Copy link
Author

fl3pp commented Nov 22, 2018

I fixed it for myself using a small plugin

https://gist.github.com/jflepp/fb8e46369ab88907020fbd143fa2b8b5

This is for sure not the best solution, but it solves my problem for now

PhrozenByte added a commit that referenced this issue Oct 20, 2019
@PhrozenByte PhrozenByte mentioned this issue Oct 20, 2019
11 tasks
PhrozenByte added a commit that referenced this issue Nov 3, 2019
```
* [New] Add `assets_dir`, `assets_url` and `plugins_url` config params
* [New] Add `%config.*%` Markdown placeholders for scalar config params and the
        `%assets_url%`, `%themes_url%` and `%plugins_url%` placeholders
* [New] Add `content-sample/theme.md` for theme testing purposes
* [New] Introduce API versioning for themes and support theme-specific configs
        using the new `pico-theme.yml` in a theme's directory; `pico-theme.yml`
        allows a theme to influence Pico's Twig config, to register known meta
        headers and to provide defaults for theme config params
* [New] Add `assets_url`, `themes_url` and `plugins_url` Twig variables
* [New] Add `pages` Twig function to deal with Pico's page tree; this function
        replaces the raw usage of Pico's `pages` array in themes
* [New] Add `url` Twig filter to replace URL placeholders (e.g. `%base_url%`)
        in strings using the new `Pico::substituteUrl()` method
* [New] Add `onThemeLoading` and `onThemeLoaded` events
* [New] Add `debug` config param and the `Pico::isDebugModeEnabled()` method,
        cehcking the `PICO_DEBUG` environment variable, to enable debugging
* [New] Add new `Pico::getNormalizedPath()` method to normalize a path; this
        method should be used to prevent content dir breakouts when dealing
        with paths provided by user input
* [New] Add new `Pico::getUrlFromPath()` method to guess a URL from a file path
* [New] Add new `Pico::getAbsoluteUrl()` method to make a relative URL absolute
* [New] #505: Create pre-built `.zip` release archives
* [Fixed] #461: Proberly handle content files with a UTF-8 BOM
* [Changed] Introduce API version 3
* [Changed] Rename `theme_url` config param to `themes_url`; the `theme_url`
            Twig variable and Markdown placeholder are kept unchanged
* [Changed] Update to Parsedown Extra 0.8 and Parsedown 1.8 (both still beta)
* [Changed] Enable Twig's `autoescape` feature by default; outputting a
            variable now causes Twig to escape HTML markup; Pico's `content`
            variable is a notable exception, as it is marked as being HTML safe
* [Changed] Rename `prev_page` Twig variable to `previous_page`
* [Changed] Mark `markdown` and `content` Twig filters as being HTML safe
* [Changed] Add `$singleLine` param to `markdown` Twig filter as well as the
            `Pico::parseFileContent()` method to parse just a single line of
            Markdown input
* [Changed] Add `AbstractPicoPlugin::configEnabled()` method to check whether
            a plugin should be enabled or disabled based on Pico's config
* [Changed] Deprecate the use of `AbstractPicoPlugin::__call()`, use
            `PicoPluginInterface::getPico()` instead
* [Changed] Update to Twig 1.36 as last version supporting PHP 5.3, use a
            Composer-based installation to use a newer Twig version
* [Changed] Add `$basePath` and `$endSlash` params to `Pico::getAbsolutePath()`
* [Changed] Deprecate `Pico::getBaseThemeUrl()`
* [Changed] Replace various `file_exists` calls with proper `is_file` calls
* [Changed] Refactor release & build system
* [Changed] Improve PHP class docs
* [Changed] Various small improvements
* [Removed] Remove superfluous `base_dir` and `theme_dir` Twig variables
* [Removed] Remove `PicoPluginInterface::__construct()`
```
PhrozenByte added a commit that referenced this issue Nov 4, 2019
```
* [New] Add `assets_dir`, `assets_url` and `plugins_url` config params
* [New] Add `%config.*%` Markdown placeholders for scalar config params and the
        `%assets_url%`, `%themes_url%` and `%plugins_url%` placeholders
* [New] Add `content-sample/theme.md` for theme testing purposes
* [New] Introduce API versioning for themes and support theme-specific configs
        using the new `pico-theme.yml` in a theme's directory; `pico-theme.yml`
        allows a theme to influence Pico's Twig config, to register known meta
        headers and to provide defaults for theme config params
* [New] Add `assets_url`, `themes_url` and `plugins_url` Twig variables
* [New] Add `pages` Twig function to deal with Pico's page tree; this function
        replaces the raw usage of Pico's `pages` array in themes
* [New] Add `url` Twig filter to replace URL placeholders (e.g. `%base_url%`)
        in strings using the new `Pico::substituteUrl()` method
* [New] Add `onThemeLoading` and `onThemeLoaded` events
* [New] Add `debug` config param and the `Pico::isDebugModeEnabled()` method,
        cehcking the `PICO_DEBUG` environment variable, to enable debugging
* [New] Add new `Pico::getNormalizedPath()` method to normalize a path; this
        method should be used to prevent content dir breakouts when dealing
        with paths provided by user input
* [New] Add new `Pico::getUrlFromPath()` method to guess a URL from a file path
* [New] Add new `Pico::getAbsoluteUrl()` method to make a relative URL absolute
* [New] #505: Create pre-built `.zip` release archives
* [Fixed] #461: Proberly handle content files with a UTF-8 BOM
* [Changed] Introduce API version 3
* [Changed] Rename `theme_url` config param to `themes_url`; the `theme_url`
            Twig variable and Markdown placeholder are kept unchanged
* [Changed] Update to Parsedown Extra 0.8 and Parsedown 1.8 (both still beta)
* [Changed] Enable Twig's `autoescape` feature by default; outputting a
            variable now causes Twig to escape HTML markup; Pico's `content`
            variable is a notable exception, as it is marked as being HTML safe
* [Changed] Rename `prev_page` Twig variable to `previous_page`
* [Changed] Mark `markdown` and `content` Twig filters as being HTML safe
* [Changed] Add `$singleLine` param to `markdown` Twig filter as well as the
            `Pico::parseFileContent()` method to parse just a single line of
            Markdown input
* [Changed] Add `AbstractPicoPlugin::configEnabled()` method to check whether
            a plugin should be enabled or disabled based on Pico's config
* [Changed] Deprecate the use of `AbstractPicoPlugin::__call()`, use
            `PicoPluginInterface::getPico()` instead
* [Changed] Update to Twig 1.36 as last version supporting PHP 5.3, use a
            Composer-based installation to use a newer Twig version
* [Changed] Add `$basePath` and `$endSlash` params to `Pico::getAbsolutePath()`
* [Changed] Deprecate `Pico::getBaseThemeUrl()`
* [Changed] Replace various `file_exists` calls with proper `is_file` calls
* [Changed] Refactor release & build system
* [Changed] Improve PHP class docs
* [Changed] Various small improvements
* [Removed] Remove superfluous `base_dir` and `theme_dir` Twig variables
* [Removed] Remove `PicoPluginInterface::__construct()`
```
PhrozenByte added a commit that referenced this issue Nov 4, 2019
```
* [New] Add `assets_dir`, `assets_url` and `plugins_url` config params
* [New] Add `%config.*%` Markdown placeholders for scalar config params and the
        `%assets_url%`, `%themes_url%` and `%plugins_url%` placeholders
* [New] Add `content-sample/theme.md` for theme testing purposes
* [New] Introduce API versioning for themes and support theme-specific configs
        using the new `pico-theme.yml` in a theme's directory; `pico-theme.yml`
        allows a theme to influence Pico's Twig config, to register known meta
        headers and to provide defaults for theme config params
* [New] Add `assets_url`, `themes_url` and `plugins_url` Twig variables
* [New] Add `pages` Twig function to deal with Pico's page tree; this function
        replaces the raw usage of Pico's `pages` array in themes
* [New] Add `url` Twig filter to replace URL placeholders (e.g. `%base_url%`)
        in strings using the new `Pico::substituteUrl()` method
* [New] Add `onThemeLoading` and `onThemeLoaded` events
* [New] Add `debug` config param and the `Pico::isDebugModeEnabled()` method,
        cehcking the `PICO_DEBUG` environment variable, to enable debugging
* [New] Add new `Pico::getNormalizedPath()` method to normalize a path; this
        method should be used to prevent content dir breakouts when dealing
        with paths provided by user input
* [New] Add new `Pico::getUrlFromPath()` method to guess a URL from a file path
* [New] Add new `Pico::getAbsoluteUrl()` method to make a relative URL absolute
* [New] #505: Create pre-built `.zip` release archives
* [Fixed] #461: Proberly handle content files with a UTF-8 BOM
* [Changed] Introduce API version 3
* [Changed] Rename `theme_url` config param to `themes_url`; the `theme_url`
            Twig variable and Markdown placeholder are kept unchanged
* [Changed] Update to Parsedown Extra 0.8 and Parsedown 1.8 (both still beta)
* [Changed] Enable Twig's `autoescape` feature by default; outputting a
            variable now causes Twig to escape HTML markup; Pico's `content`
            variable is a notable exception, as it is marked as being HTML safe
* [Changed] Rename `prev_page` Twig variable to `previous_page`
* [Changed] Mark `markdown` and `content` Twig filters as being HTML safe
* [Changed] Add `$singleLine` param to `markdown` Twig filter as well as the
            `Pico::parseFileContent()` method to parse just a single line of
            Markdown input
* [Changed] Add `AbstractPicoPlugin::configEnabled()` method to check whether
            a plugin should be enabled or disabled based on Pico's config
* [Changed] Deprecate the use of `AbstractPicoPlugin::__call()`, use
            `PicoPluginInterface::getPico()` instead
* [Changed] Update to Twig 1.36 as last version supporting PHP 5.3, use a
            Composer-based installation to use a newer Twig version
* [Changed] Add `$basePath` and `$endSlash` params to `Pico::getAbsolutePath()`
* [Changed] Deprecate `Pico::getBaseThemeUrl()`
* [Changed] Replace various `file_exists` calls with proper `is_file` calls
* [Changed] Refactor release & build system
* [Changed] Improve PHP class docs
* [Changed] Various small improvements
* [Removed] Remove superfluous `base_dir` and `theme_dir` Twig variables
* [Removed] Remove `PicoPluginInterface::__construct()`
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants