Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gogowitsch created an issue. See original summary.

Gogowitsch’s picture

Gogowitsch’s picture

To test this, I used a my_module_name.routing.yml file and added a row with just a single : on it.

jhedstrom’s picture

Issue tags: +Needs tests

This looks like a great DX win!

I think a few quick unit test cases could be added to the existing \Drupal\Tests\Component\Discovery\YamleDiscoveryTest to ensure the expected exception messages are thrown for the 2 added exceptions.

jhedstrom’s picture

Status: Needs review » Needs work

Setting to NW for tests.

Gogowitsch’s picture

Thanks for the nudge. I have added a test to see if the expected exception is thrown and if it contains the filename.

Now I just need to find the upload button for my new patch - why is the d.o UI different today?

Gogowitsch’s picture

Ah, the upload is not offered for the first comment directly after logging in. Sorry for the noise, patch is attached.

Status: Needs review » Needs work

The last submitted patch, 7: 3046298-6.drupal.DX-tell-developer-which-yaml-file-caused-a-ParseException.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jhedstrom’s picture

+++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDiscoveryTest.php
@@ -2,11 +2,12 @@
+use Drupal\Core\Discovery\YamlDiscovery;

The test is failing because this is using a class from core in a component:

1) Drupal\Tests\Component\DrupalComponentTest::testNoCoreInComponentTests
Checking for illegal reference to 'Drupal\Core' namespace in /var/www/html/core/tests/Drupal/Tests/Component/Discovery/YamlDiscoveryTest.php
Failed asserting that an array is empty.

There is also a YamlDiscovery class in the component namespace, so if you switch to using that instead, the tests should go green.

Gogowitsch’s picture

Changes since the last patch:

  1. Added newlines to fix the 2 coding standards messages
  2. Reverted changes to Component/Discovery/YamlDiscoveryTest.php except for alphabetic use statements
  3. Added new test in the Core instead of the Component namespace
Gogowitsch’s picture

Status: Needs work » Needs review

Tests pass, so this issue now "Needs review".

jhedstrom’s picture

The issue with Core vs Component test issue got me digging into the Component implementation and I realized it has the same DX issue.

  1. +++ b/core/lib/Drupal/Core/Discovery/YamlDiscovery.php
    @@ -16,7 +18,15 @@ class YamlDiscovery extends ComponentYamlDiscovery {
       protected function decode($file) {
    -    return Yaml::decode(file_get_contents($file)) ?: [];
    +    try {
    +      return Yaml::decode(file_get_contents($file)) ?: [];
    +    }
    +    catch (ParseException $e) {
    +      throw new ParseException($file . ': ' . $e->getMessage(), $e->getParsedLine(), $e->getSnippet(), $file, $e);
    +    }
    +    catch (InvalidDataTypeException $e) {
    +      throw new InvalidDataTypeException($file . ': ' . $e->getMessage(), $e->getCode(), $e);
    

    I think this bit of logic should also go into \Drupal\Component\Discovery\YamlDiscovery::decode(), and then your previous test for the component can be added back, and we'll have a test for both core and component versions, and fix the DX issue in both places.

    Sorry I didn't catch that in a previous round of feedback.

    The rest looks good to go!

Gogowitsch’s picture

Yeah, I agree. Thanks for your review!

In comparison to the last patch, the new one fixes the DX issue also on the Component side.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

This looks great! Thanks for sticking with all the reviews :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3046298-13.drupal.DX-tell-developer-which-yaml-file-caused-a-ParseException.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

voleger’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.54 KB
476 bytes

Just CS fix, back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3046298-16.patch, failed testing. View results

Gogowitsch’s picture

Status: Needs work » Reviewed & tested by the community

The patch from #16 looks good to me.

Tests are green now. As I see no relationship between our changes here and LayoutBuilderDisableInteractionsTest, I attest the previous failure to a random flakiness. Other opinions are welcome.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Discovery/YamlDiscovery.php
@@ -16,7 +18,15 @@ class YamlDiscovery extends ComponentYamlDiscovery {
+      throw new ParseException($file . ': ' . $e->getMessage(), $e->getParsedLine(), $e->getSnippet(), $file, $e);

There doesn't appear to be any test coverage for this, only for the invalid data type exception.

we should be adding that here if we want this feature to stick around

Other than that, this is a great feature, would save so much pain - thanks!

Gogowitsch’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.82 KB
2.76 KB

There is currently no Yaml::* call within Drupal that will throw a ParseException. The Symfony component solely throws ParseExceptions, and the 2 existing Drupal wrappers in core solely throw InvalidDataTypeExceptions.

Changes since the last patch:

  • Removed code to catch the wrong exception type. Luckily that means we don't need to write a test for code that no longer exists.
  • SerializationInterface: documented that encode and decode may throw an InvalidDataTypeException.
alexpott’s picture

Category: Feature request » Task
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

This is line with other parts of the system for example - \Drupal\Core\Asset\LibraryDiscoveryParser::parseLibraryInfo() and looks great. Nice work.

https://www.drupal.org/files/issues/2019-06-05/3046298-20.drupal.DX-tell...

  • alexpott committed 45f97bc on 8.8.x
    Issue #3046298 by Gogowitsch, voleger, jhedstrom, larowlan: DX: tell...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.