Inconsistent parsing of version.yaml

[tobias-kuendig]

A problem did arise with our mall plugin's version.yaml. Suddenly, this line is causing an issue during the installation:

https://github.com/OFFLINE-GmbH/oc-mall-plugin/blame/640ae7f236c9290678e2cceb99b4cf764b15fd98/updates/version.yaml#L195

Issue OFFLINE-GmbH/oc-mall-plugin#573

Because of the : in the message, this is interpreted as an array. I guess according to the YAML spec this is the correct thing to do and the message should have been wrapped in quotes to begin with.

The strange thing is, this has been working for the past year without any problems. It currently still works in the plugin's backend detail page (/backend/system/updates/details/offline-mall/changelog) and the plugin's marketplace page.

But it fails when installing the plugin on the latest version of October.

For one, it would be great if someone from the October team could manually patch the plugin's changelog since the fixed changelog I publish via the Marketplace does not override old entries:
https://github.com/OFFLINE-GmbH/oc-mall-plugin/pull/572/files

Apart from that, is there anything that should be patched? This behaviour clearly changed in one of the last few releases. The relevant lines in the VersionManager class have not been changed recently:
https://github.com/octobercms/october/blame/1.0/modules/system/classes/VersionManager.php#L563

What happens if someone releases a version.yaml with the same "problem"? This would also break their plugin.

[LukeTowers]

@tobias-kuendig if you release a new version then your plugin will be rebuilt, the cached copy of the changelog on the marketplace shouldn't cause any issues with the actual installation of the plugin itself if you trigger a rebuild by releasing a new version.

Seems like this is a new warning bubbling up from newer PHP versions.

[tobias-kuendig]

if you release a new version then your plugin will be rebuilt,

That works, thanks. I must have messed up something on the first try as the error was still there.

Seems like this is a new warning bubbling up from newer PHP versions.

That's what I thought at first but it looks like this warning has been there since the early days: https://3v4l.org/MeoBt

It looks like it is related to the symfony/yaml component. Downgrading symfony/yaml from v3.4.44 to v3.4.43 makes the issue disappear. But to me there is no obvious line that caused this change:
symfony/yaml@v3.4.43...v3.4.44

Parsed versions.yaml line in 3.4.44:

array:1 [
  "!!! This release does not contain any code updates. (...) website" => "https://bit.ly/2Y4lUs3"
]

Output with 3.4.43:

  "!!! This release does not contain any code updates. (...) website: https://bit.ly/2Y4lUs3"

What do you think, do we have to do something about that? It's an edge case and the YAML was invalid to begin with.

[LukeTowers]

@tobias-kuendig I think at very least it's worth reporting to Symfony, perhaps they have some insight into why a patch version suddenly started parsing differently.

[tobias-kuendig]

It looks like the real problem is the !!! part of the changelog message.

According to the yaml spec, a line starting with ! has a special meaning:
https://stackoverflow.com/questions/9664113/what-does-a-single-exclamation-mark-do-in-yaml

I'm not sure if it has to explicitly be a single ! or if !!! counts as a "non-specific tag" as well.

Maybe we should make sure that these messages are properly quoted in the docs? https://octobercms.com/docs/plugin/updates#version-file

[bennothommo]

@tobias-kuendig I've added some unit tests for this here: e7b1862

It seems to be the combination of both !!! and the : in the middle of the string which is doing this. Using !!! on its own seems to have no issue.

I think you're right - we'll have to update the docs to stipulate quotes around the message in cases like these. We could potentially lock symfony/yaml to v3.4.43, but they are right in the sense that 3.4.44 brings it closer to the Yaml spec.

[tobias-kuendig]

Check out the updated symfony issue as well:
symfony/symfony#38154 (comment)

I guess using quotes for the update messages is the proper way forward.

[LukeTowers]

@bennothommo @tobias-kuendig is there anything we can do (perhaps pre-process the file) to fix this for October users? Are we able to preprocess the file to automatically add quotes around the !!! line?

[bennothommo]

@LukeTowers we certainly can try and implement some pre-processing to catch most of the issues - for example, if we pre-process the version numbers to contain quotes, we could even upgrade symfony/yaml to 4.x - we had to lock it to 3.x for L6 as 4.x didn't support our version numbers, but 3.x is EOL next year.

The only potential concern is some fringe cases where people may have formatted their YAML files in weird ways - 3.x definitely seems to be more forgiving than later versions.

[LukeTowers]

I figure we can probably just stay on 3.x for a while, there's not much benefit from upgrading AFAIK.

[github-actions]

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

[bennothommo]

I encountered this a couple of days ago when I was testing something else - I'll have to put some further thought into mitigating this.

[josephcrowell]

It also doesn't parse correctly if tabs are used instead of spaces.

[LukeTowers]

@josephcrowell that won't be changed. YAML does not support tabs and never will. See https://yaml.org/faq.html

[slowpokefarm]

I figure we can probably just stay on 3.x for a while, there's not much benefit from upgrading AFAIK.

since we are now on laravel 6 some laravel packages require never version of symfony/yaml. Can we expect to upgrade it to 4.1 at least any time soon?

@LukeTowers

[LukeTowers]

@slowpokefarm you can try if you'd like, but it's going to be tricky to make it work because they made some breaking changes that impact October quite heavily as I recall. You could also try the as or replace composer tricks to resolve those issues.

[bennothommo]

@slowpokefarm We cannot upgrade symfony/yaml to 4.x at this time as those versions of the library no longer read our version.yaml files. The main problem is that it won't accept our version numbers 1.0.0: or 1.0.1: as an array key unless it is surrounded by quotes, but October has traditionally used them without quotes.

We would have to pre-process the YAML files in some way before running them through the Yaml library if we wanted to upgrade it, as @LukeTowers suggested here.

[slowpokefarm]

@bennothommo Hello, I understand the issue but like @LukeTowers already mentioned 3.x is approaching its EOL this year and is already marked unmaintained. While 6.x laravel packages tend to require symfony/yaml of version 4.x and 5.x this issue deprives october cms of one of its strongest advantages - laravel packages compatibility . We can't keep 3.x forever just because we have issues with inconsistent yaml format we use in our versions config.
I believe we need to find some way to implement pre-processing of versions and/or begin to migrate them to valid yaml spec.
Maybe we also need a separate Issue for that to discuss our options or ideas.

[LukeTowers]

@slowpokefarm We both agree with you, no need for a separate issue, this is the issue for it. From where we are right now the way forward is preprocessing, or perhaps processing-on-exception, with either solution implementing some form of caching potentially (I think there may already be some caching of processed YAML, we could probably tweak that).

It's not a pressing issue for either myself or @bennothommo at the moment, so if it is for you feel free to work on a PR to implement support for newer versions of the YAML package.

[daftspunk]

As discussed integer keys will no longer be supported soon. I've updated the docs to reflect this octobercms/docs@db8fd73

This is a proactive measure for what's coming but all plugin and theme authors should look at updating their version.yaml files to remain compatible in the future

edit: On second pass, the proposed YAML is quite messy and hard to read (defeating the purpose). The Builder plugin also seems to output "invalid YAML" even though it is produced by the Symfony\Component\Yaml\Dumper. It would make more sense to adjust the values instead in a major release

[slowpokefarm]

So, what is the final decision on this?

[slowpokefarm]

As discussed integer keys will no longer be supported soon. I've updated the docs to reflect this octobercms/docs@db8fd73

This is a proactive measure for what's coming but all plugin and theme authors should look at updating their version.yaml files to remain compatible in the future

edit: On second pass, the proposed YAML is quite messy and hard to read (defeating the purpose). The Builder plugin also seems to output "invalid YAML" even though it is produced by the Symfony\Component\Yaml\Dumper. It would make more sense to adjust the values instead in a major release

@daftspunk Hi there again. Shall we expect the changes for the 1.1 or will it be just left out there without proper support for laravel 6 packages? Maybe it's a good idea to rollback to laravel 5.5 then since old version of packages are fine with symfony/yaml version 3.x.