Unable to parse when page meta has no "space after the two dots"

[AeonFr]

I accidentaly broke my site for saving a wrong file. Simply putting Author:Example instead of Author: Example and the hole site becames unaccesible. This is what PHP throws:

Fatal error: Uncaught exception 'Symfony\Component\Yaml\Exception\ParseException' with message 'Unable to parse at line 2 (near "Author:Francisco Cano").' in ***/pico/vendor/symfony/yaml/Parser.php:298 Stack trace: #0 ***/pico/lib/Pico.php(764): Symfony\Component\Yaml\Parser->parse('Title: Ayuda\nAu...') #1 ***/pico/lib/Pico.php(958): Pico->parseFileMeta('/*\nTitle: Ayuda...', Array) #2 ***/pico/lib/Pico.php(337): Pico->readPages() #3 ***/pico/index.php(31): Pico->run() #4 {main} thrown in ***/pico/vendor/symfony/yaml/Parser.php on line 298

[PhrozenByte]

Yes, this is a known problem. Unfortunately I've absolutely no idea how to work around this: What behavior would you expect? Silently ignoring it? The result would be that this page has no meta data, so e.g. the page title is empty (we could at least fallback to the filename), but I think hiding such problems makes the situation worse.

[AeonFr]

I have the feeling this has something to do with the regex in parseFileMeta() function in lib/Pico.php

public function parseFileMeta($rawContent, array $headers)
    {
        $meta = array();
        $pattern = "/^(\/(\*)|---)[[:blank:]]*(?:\r)?\n"
            . "(.*?)(?:\r)?\n(?(2)\*\/|---)[[:blank:]]*(?:(?:\r)?\n|$)/s";
        # ...
   }

I can't understand a single character of that regex but it was in the stack trace.. :P

The problem broke my hole site, even the editor plugin. It would have been better if only that page would break (well, at least I could log in to picoeditor and fix it through the browser)
I guess for now is just a matter of being careful about it

Thanks for the answer 🙌

[PhrozenByte]

No, it's not the regex, the exception is thrown by the Symfony YAML parser. Any thoughts about the expected behavior (also @theshka)?

[theshka]

My opinion is that Key:Value is not a valid header, and it should throw the exception. Perhaps we could clarify the importance of having the space Key: Value in the header to avoid mistakes like this somewhere in the user documentation @PhrozenByte ?

P.S. @AeonFr, if you add something like syntax highlighting javascript to the editor, it may help to visualize the error before you save.

[mayamcdougall]

Personally, I think that Pico should throw more error messages instead of just crashing silently. In it's current design, it makes it really hard to debug issues if you've made more than one change or changed more than one file.

I also think it could be beneficial from a theme developing perspective too. I don't know if it would be possible, but messages like "Error parsing twig template" or "Twig error: malformed expression" would be immensely helpful. I can't tell you how many times I've broken my theme with malformed logic and gone "which one of those five changes was the one that broke it?".

For this issue, a message of "Error parsing file header: filename" would be much better than a blank page, though it wouldn't help with the issue of the Editor not working.

What about ignoring files with malformed headers? Not ignoring the metadata, but the whole file. Although still silent, the fact that their file isn't showing up would be an indication in itself that something is wrong with it. This would allow the site (and editor) to keep functioning, but wouldn't be "hiding" the problem.

I don't know, I just really really hate the idea of content files being able to bring down a site. A messed up theme or plugin, sure, but not a content file.

[PhrozenByte]

@theshka: I don't think that this was on purpose, more a typo. Anyway, we should link to the YAML specs somewhere, like we already do for Markdown and Twig. I've added this in 6cb378e.

I personally don't want to add comprehensive error handling to Pico, it increases complexity without any advantage for ordinary users. Plus, there's no big difference between showing an exception (or a white page on production systems with disabled display_errors - this is intended!) and a "nice" fatal error page.

This doesn't mean that we shouldn't work around recoverable (non-fatal) errors, especially errors which occur while normal usage (like this one). As far as I can tell at the moment, we have three options:

1. Do nothing, just keep throwing the exception
2. See Catch YAML parse errors #293
3. See Catch YAML parse errors (2) #294

What do you think?

[theshka]

If we are going to work around the recoverable error, 2/#293 has my vote, otherwise I'd say just throw the exception. 😐

[mayamcdougall]

Maybe something in-between? #293, but check if the theme contains the new 'YAML_ParseError' variable. If it doesn't, display a blank (or slightly styled) page with the contents 'YAML_ParseError'. This would provide a nice error message, instead of throwing an exception, even if the theme didn't include error support.

Also, should the new variable be specific to YAML errors, or could it be used to catch other errors and therefore be called something different (like just "ParseError")? I'm not sure if there are any other non-fatal error types that could be caught this way, it was just something I thought of.

[mayamcdougall]

@PhrozenByte Also, is there a way to display_errors instead of a blank page? I had no idea... 😓

If there isn't a way to turn that on in the config.php, there really should be.

[PhrozenByte]

This would heavily increase complexity, therefore: No. display_errors is a php.ini config, this is non of our business 😉

[mayamcdougall]

Ah, okay. The way you said it made me think there was some secret Pico setting I didn't know about. 😆

[theshka]

I misunderstood, I would like to actually merge #294; it will show the exception but still allow you to browse to the admin plugin, etc... As you mentioned more expansive error handling is added complexity, and I think if we start going down the "nice" error message route(i.e. #293) this is "puling at the thread" so-to-speak... (even though I initially like it better)

[AeonFr]

Yes, I'm all for that change, I'll try implementing it myself!.
I'm also discovering that the [ character is ilegal inside a meta header if it's the first character. (ej: meta: [). It's good to now where the error is, from the PHP error message, but It would be much nicer if all the other pages remain accessible!
Thanks for the answers, btw!

[PhrozenByte]

Fixed with the merge of #294. Thanks @AeonFr for reporting! 👍

Also thanks @theshka and @smcdougall for giving feedback!