platformsh/legacy-cli

InvalidConfigException can not parse error when CommandNotFoundException triggered

Closed this issue · 8 comments

gilzow commented

we have our platform.app.yaml file broken up into numerous sub yaml files that are included (this allows us to organize sections that downstream projects should alter, vs sections they should not). With the release of the CLI tool v3.52, we now get an InvalidConfigException, unable to parse error whenever a CommandNotFoundException is raised.

For example, platform help environment will cause a [InvalidConfigException] Unable to parse at line error, whereas platform help environment:list does not produce any errors. Additionally, the platform environment itself is able to parse the yaml files without issue.

It appears that the yaml parser being used post v3.51 does not like the keys to be indented in included yaml files, at least when raising another exception. v3.51.x and earlier, did not exhibit this behavior.

pjcdawkins commented

Could you show an example of what you mean by keys being indented?

There's a test for a whole file being indented, and the test is passing:
https://github.com/platformsh/platformsh-cli/blob/master/tests/Util/YamlParserTest.php#L42-L60

gilzow commented

sure!
This is from our dependencies.php.yaml file which is included from the platform.app.yaml file via

dependencies:
  php: !include project/platform/dependencies.php.yaml

###
# Contains all of the php dependencies the _environment_ needs
###
################################
###          CORE           ###
###############################
# reserved

################################
###        WORDPRESS        ###
###############################
    # Install wp cli and its bundles
    wp-cli/wp-cli: "^2.1"
    wp-cli/wp-cli-bundle: "^2.1"
    wp-cli/find-command: "dev-master"
    psy/psysh: "^0.8.4"
pjcdawkins commented

Reproduced, it looks like the parser [now] wants the comments to be indented by the same amount as the keys.

While philosophically that seems wrong to me (because comments should not affect surrounding code), and a fix seems fairly doable, I also don't want to diverge too far from Symfony's parser as it's by far the main way to parse YAML in PHP. Is there a strong reason to keep the indents like this?

gilzow commented

Strong reason? no. It'll take us a little bit to get the change propagated out to our 100+ projects, but as long as you think the issue is isolated to just this scenario, I can educate developers to avoid this situation (or just know the issue can occur) until they merge from our upstream.

pjcdawkins commented

I think the parser change responsible may be from Symfony v3.4.31:
symfony/symfony#32767

If so, this was the case since the platformsh-cli v3.48.0, released on 21 September 2019.

pjcdawkins commented

Actually testing with the platformsh-cli v3.47.0 (and symfony v3.4.27), I still get the same parse error.

Maybe this CommandNotFoundException is triggering the parse error in a different way with more recent CLI versions, but I don't see that this could have been parsed in recent CLI versions.

pjcdawkins commented

OK the exception is probably being thrown because the command list is getting loaded, which triggers isHidden() on every command, which means that the app config is getting parsed via:

https://github.com/platformsh/platformsh-cli/blob/master/src/Command/Environment/EnvironmentXdebugCommand.php#L33-L65

It would make sense not to throw an exception in that case.

gilzow commented

I'll still begin work on removing indentation from included yaml files. It's been awhile since we broke up the platform.app.yaml file into multiple includes, and I'm betting the indentation was just left from what it used to be in platform.app.yaml.

  • 👍1