[YAML] Add support to parse and dump comments

[regniblod]

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	?
RFC?	no
Symfony version	latest

As is stated in the official docs, the YAML component is not able to parse and dump comments. I need to parse and dump comments in a helper command I'm doing to add Doctrine mappings automatically for new bundles into the config.yml file for Symfony Standard distribution (so I need to keep the comments).

[xabbuh]

I am not convinced that we should add this feature. The parser is already complex enough. So 👎 from me.

[fabpot]

Definitely not something that we want to implement.

[peterjernei]

I understand your point, but still, this could be useful feature in some cases.

At the moment we are storing configuration settings in YAML files, where i would like to keep the comments added above the key-value pairs. I would like to have these comments added when we are writing out the bean, so no parser changes needed. I would like to have something like an extension point where i can add my custom property scanner that checks if the field has an annotation like this:
`@YamlComment("The comment to be added in the dump to this field")

private String field = "value";`

And the dump should look like this:
`# The comment to be added in the dump to this field

field: value`

Or please let me know if it is already implemented. I think this comment annotation would be used frequently by other users also.

[TomasVotruba]

Why not just make it optionally extendable for those who need it? Like Twig filters, that anyone can add and don't have to be in the core.

[xabbuh]

Even "just" adding an extension point increases the complexity of the parser. Feel free to give it a try, but I doubt this will be possible without too much unneeded complexity.

[TomasVotruba]

I don't know parser nor its performance in detail, so I don't think my attempt would be useful or effective :)

But adding extension point != increase complexity. I often add such point to decrease complexity and lower responsbility.

[xabbuh]

Well, I don't see how that could be done in this specific case without adding too much complexity. So yes, the only thing I can offer is that someone gives it a try and proves us wrong here.

[danepowell]

I understand the complexity involved but I still think a lot of people would greatly benefit from this feature. Comments aren't just a "nice to have" in YAML files. Given that people use YAML specifically because its human-readable, I think you should treat the human-focused elements (comments) with the same priority as everything else.

I maintain BLT, which has some automation tools to help people manage large sets of YAML files. Right now we have to force users to choose between taking advantage of that all of that helpful tooling or keeping their comments. It's a pretty terrible choice.

[danepowell]

Also if anyone is looking to handle comments without the help of Symfony, here are some other tools (I can't comment on their quality):

• https://github.com/consolidation/comments
• https://github.com/klausi/yaml_comments

[gogowitsch]

@danepowell Here comes my feedback to the tools you mentioned:

• https://github.com/consolidation/comments works fine except if the comment is under a key that is also found elsewhere. For example for Drupal routing files, it got confused about comments in the requirements: key and moved them to another route.
• https://github.com/klausi/yaml_comments doesn't have a way to write comments back (and is archived).

I also checked if https://github.com/mustangostang/spyc can write comments back - not at this time.

[lstrojny]

Since the decision to not support this is three years old now, maybe reality has changed enough to reconsider this:

• Since SF is going to recommend PHP config files going forward, tooling that automates that step would greatly benefit from a comment preserving YAML parser. Independent of whether this tooling comes directly from Symfony or is some community effort, preserving comments would be an important enabler here.
• As Symfony’s YAML flavor is basically unparseable with non-Symfony parsers, the ecosystem around Symfony cannot build common productivity tools like auto formatters or refactoring tools (case in point https://github.com/migrify/migrify/issues/79) for config files without losing comments. Not having comments should still be JSONs job, AMIRITE?

[fabpot]

"As Symfony’s YAML flavor is basically unparseable with non-Symfony parsers" -> That's new to me.

[lstrojny]

"As Symfony’s YAML flavor is basically unparseable with non-Symfony parsers" -> That's new to me.

Various parsers won’t accept e.g. the !php/const syntax (or interpret it strangely), e.g.

bar: !php/const PHP_INT_MAX

[fabpot]

But that's valid YAML AFAIK. But if you're not using this (I won't use it myself), the YAML parser/dumper IS YAML compliant and should be parseable in other languages (I'm doing that in Go without any problems).

I don't really get the goal of your comment to be honest.

[lstrojny]

I feel we are applying different definitions for what we consider compatible but it probably doesn’t matter terribly much.

The goal of my original comment was to reopen the discussion on should Symfony’s YAML parser have a comment preserving mode and my argument for why it should have one is because it would enable a whole class of tooling to exist that likely won’t exist if the parser does not support it.

[gharlan]

Let's say there would be such comment preserving mode, what would be the expected result for this yaml document:

# comment 1
root: # comment 2
    foo: 1
    # comment 3
    bar: 2 # comment 4
    baz: 3

How would you represent this structure after parsing?

[Chi-teck]

@gharlan The parsed YML would need to be represented as object like DOM.
Something like this.

$yml->getValue(); // ['root' => ['foo' => 1, 'bar' => 2, 'baz' => 3]]
$yml->root->bar->getValue(); // 2
$yml->root->bar->getComment(); // 'comment 3'
$yml->root->bar->getInlineComment(); // 'comment 4'

[Chi-teck]

One more closely tied task is preserving original formatting of Yaml file. That's really needed for code generators to merge generated Yaml with existing one.

[donquixote]

I've been thinking about this.
An interesting solution could be patching. Perhaps not as part of symfony yaml package, though.

The following steps would be automated and called from php with exec:

• Read and re-export the original data to a new tmp file, destroying comments and custom formatting.
• Generate a diff from new to old, save as a patch.
• Export the modified data.
• Apply the patch to the modified file. Use --merge to allow human conflict resolution.

[shaedrich]

I understand the complexity involved but I still think a lot of people would greatly benefit from this feature. Comments aren't just a "nice to have" in YAML files. Given that people use YAML specifically because its human-readable, I think you should treat the human-focused elements (comments) with the same priority as everything else.

I maintain BLT, which has some automation tools to help people manage large sets of YAML files. Right now we have to force users to choose between taking advantage of that all of that helpful tooling or keeping their comments. It's a pretty terrible choice.

I totally agree. We are not talking about a custom feature request, this is a valid native "type". So refusing to even have basic functionality built in, is a bit weird. In other words, using automation tools, this becomes unusable if you don't want to unnecessarily annoy your users. It's a shame that something as big as symfony can't offer basic functionality while on the other hand has stuff like custom tags that aren't necessary for providing basic functionality.

[xabbuh]

Obviously nobody cared enough about this to come up with a PR proving that adding this would be possible without making the parser even more complex than it is right now.

[stof]

Parsing into an AST is what is necessary to be able to have comments preserved. This is a totally different kind of parsing than parsing the data themselves.

This is not a feature we can "simply" add. It involves a full rewrite of the component.

Also, I would not consider "parse into an AST" the basic functionality either.

[Chi-teck]

AST parsing might also have a performance penalty. This may impact projects that use this component for loading configuration run-time.
I think the new parser should be independent from existing one.

[stof]

Indeed. Producing an AST first and then resolving the data from the AST will indeed likely be slower (and consume more memory) than directly parsing the data.
So to me, this should be left to a separate project.