Problem/Motivation

What are the steps required to reproduce the bug?
Do something that requires Drupal to process a YAML file which only contains comments.
In my case it was installing a theme which has multiple YAML files, one of which holds a commented out example configuration.

Do so in an environment which has PECL YAML installed, otherwise Drupal will use the Symfony YAML component which appears to be more standards compliant for this aspect.

What behavior were you expecting?
No errors, because YAML files with only comments are valid. See http://yaml.org/spec/current.html#id2527268

What happened instead?
A PHP warning was thrown.

Drupal\Component\Serialization\Exception\InvalidDataTypeException: yaml_parse(): end of stream reached without finding document 0 in            [error]
/var/www/docroot/core/lib/Drupal/Component/Serialization/YamlPecl.php:72
Stack trace:
#0 [internal function]: Drupal\Component\Serialization\YamlPecl::errorHandler(2, 'yaml_parse(): e...', '/var/www/docro...', 54, Array)
#1 /var/www/docroot/core/lib/Drupal/Component/Serialization/YamlPecl.php(54): yaml_parse('# layout_name_g...', 0, 0, Array)

Proposed resolution

Catch the specific error message and treat it just like we already treated empty YAML files since #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available

Remaining tasks

Patch review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#8 interdiff-3003300-8.patch940 bytesswentel
#8 3003300-8.patch2.29 KBswentel
#4 pecl_yaml_only_comments_error-3003300-4.patch2.27 KBpieterdc
#2 pecl_yaml_only_comments_error-test_only-3003300-1.patch517 bytespieterdc

Comments

PieterDC created an issue. See original summary.

pieterdc’s picture

pieterdc
Primary language Dutch
Location Ghent
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new pecl_yaml_only_comments_error-test_only-3003300-1.patch517 bytes

Extended existing test to cover scope of this issue. Patch attached.

Status: Needs review » Needs work

The last submitted patch, 2: pecl_yaml_only_comments_error-test_only-3003300-1.patch, failed testing. View results

pieterdc’s picture

pieterdc
Primary language Dutch
Location Ghent
commented
Assigned: pieterdc » Unassigned
Status: Needs work » Needs review
Related issues: +#1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available
StatusFileSize
new pecl_yaml_only_comments_error-3003300-4.patch2.27 KB

Ok, so that test run proved that current Drupal core doesn't catch PECL YAML its failure to accept YAML files with only comments.
Newly attached patch implements the proposed solution from the issue description. Let's see if that fixes the extended test.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Version: 8.6.x-dev » 8.7.x-dev

Coming here from #2982684-98: Add a composer scaffolding plugin to core

Patch still applies, re-running test.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

swentel’s picture

swentel commented
Version: 8.9.x-dev » 9.3.x-dev
StatusFileSize
new 3003300-8.patch2.29 KB
new interdiff-3003300-8.patch940 bytes

Still applies fine to 9.3.x as well.

Changed the patch with some minor code styling (catch and else on a new line - unless I miss something here)

swentel’s picture

swentel commented

Cancelled the interdiff test, sorry for the noise.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

neclimdul’s picture

neclimdul
He/Him
commented

Pecl YAML fixed this over 3 years ago. Is there a reason to still apply this patch?

+++ b/core/lib/Drupal/Component/Serialization/YamlPecl.php
@@ -36,10 +36,6 @@ public static function decode($raw) {
-    // yaml_parse() will error with an empty value.
-    if (!trim($raw)) {
-      return NULL;
-    }

This isn't even valid anymore. Should probably follow up to remove it.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This will need a test case to show the issue.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Status: Needs work » Closed (outdated)
Issue tags: +Bug Smash Initiative

This was a bugsmash triage target today. Both chx and mstrlen recommend that this is closed as outdated.