Exceptions from errors in services.yml files should tell you which file

Just thinking here, but would something like this work? (just illustrating here)

      try {
        $yaml_loader->load($filename);
      }
      catch (\Exception $e) {
        \Drupal::logger('php')->error('The file ' . $filename . 'failed to load due to error: ' . $e->getMessage());
      }

I don't know if it's a good approach (and what would be a good one XD). Any help is appreciated.

Symfony now has some code to handle this: https://github.com/symfony/dependency-injection/blob/f0d7ec8b5d84e262498...

We use a custom Yaml parser (which invokes PECL or Symfony as available) so we can't copy the code exactly. But I suggest we do copy the error message from Symfony so our YamlFileLoader is closer to theirs.

@joachin You're right. We should halt the execution. Looks like it's throwing an InvalidDataTypeException atm. Should i keep it?

@longwave i like the message from symfony.

I would catch the InvalidDataTypeException and throw InvalidArgumentException, as that is used for all other exceptions in YamlFileLoader - the filename argument is considered invalid because of a parse error, in this case.

Ok so, we're catching InvalidDataTypeException here and throwing InvalidArgumentException in both load calls.

I attached 2 screenshots showing the current error message and the "new" one.

Hope it helps. Kindly review it :)

Thanks, the new message is a good improvement.

Why don't we modify YamlFileLoader instead, then we only have to catch and throw once, in the same place as Symfony does?

@longwave the reporter (@joachim) said at the issue summary:

This should be done in DrupalKernel rather than YamlFileLoader, as YamlFileLoader should be kept as close as possible to the Symfony version of that class that is is a fork of.

If you guys agree that YamlFileLoader is the best place to make the approach i can send another patch :)

I think as Symfony's YamlFileLoader added this since we copied it, it's OK to put the same (or at least very similar) code in our copy too.

Here's a new patch with the proposed approach in YamlFileLoader.

I won't upload images this time as the messages remains the same.

Hope it helps. Will keep in "needs review", kindly review it and choose whichever fits the goal better :)

Oh my... Sorry for the typo there, i was in a hurry, my mistake.

Ok so:

#13-1: Pascal case would be like $ValidFile, while camel case stands for $validFile, am i wrong? I think the variable name in this case is already camel case.

#13-2: InvalidDataTypeException doesn't need a backslash because it's already declared in an use statement under \Drupal\Component\Serialization\Exception\InvalidDataTypeException. Also thanks for pointing it, because i had used a backslash in InvalidArgumentException without noticing it was also declared in an use statement XD

#13-3: Corrected the typo, thanks for pointing it :D

New patch on it's way. Kindly review it :D

No problem haha.

This one fixes the variable name from camel case to snake case. Thanks for pointing it and for the clarification.

Kindly review it. :)

Random fail, back to RTBC.

Once again, random fail (retested it and passes). Yet, i'l leave it in needs review so that someone else checks it and moves to RTBC again if everything is ok :)

Yet another random fail in Quick Edit.

Random fail in CKEditor.

Back to RTBC as per the above comments.

+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -425,7 +426,14 @@ protected function loadFile($file)
+        try{

Just spotted that we need a space between try and {.

Added spacing around try.
thanks

Typo has been fixed, back to RTBC.

Nice work. This is a solid bugfix.

We could provide ExpectedException test coverage for it as well. Is there a reason not to?

https://www.drupal.org/pift-ci-job/2548777 also looks actually relevant.

The 'qqq' line in the patch looks like stray test code (in the sense of 'work in progress/experimental', not unit tests) that should have been removed before rolling maybe?

Definitely a case of "Last Patch of The Day"-syndrom.

Heredocs can now be indented with the surrounding code.

Tried that locally, but it doesn't seem to work with indentation-sensitive code like YAML.

Ran the test locally for #44 and it fails with

Failed asserting that exception message 'Unable to parse at line 1 (near "   do not:").' contains 'The file "vfs://drupal/modules/example/example.yml" does not contain valid YAML'.

Tried breaking an existing service but could not but the test shows the issue just fine I believe.

Looks good!

Is this a case where we should be using assertions instead of exceptions? See for example #2454649: Cache Optimization and hardening -- [PP-1] Use assert() instead of exceptions in Cache::merge(Tags|Contexts). Since there are other similar exceptions in the same file, maybe a follow-up?

Opened #3339727: YamlFileLoader use assertions instead of exceptions

Committed e53445e and pushed to 10.1.x. Thanks!