Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available

Sounds like a pretty easy Drupal wrapper to write. If the extension is available, parse the YAML with the PECL extension. If not, use Symfony's parser as a fallback.

Tagging.

Yes, lets.

It's faster, and all around better and more complete as it uses LibYAML.

I already looked into this some time ago. The primary reason for why I did not continue is:

http://www.php.net/manual/en/book.yaml.php uses http://pyyaml.org/wiki/LibYAML, which implements the YAML spec v1.1.

Symfony's Yaml component implements v1.2.

Symfony's yaml component doesn't not implement the complete spec:

https://github.com/symfony/Yaml/blob/master/README.md#yaml-component

Quick example of where symfony falls down:

php> $str = "blah: !!int 123.23";

php> = Symfony\Component\Yaml\Yaml::parse($str);
array(
  "blah" => "!!int 123.23",
)
php> = yaml_parse($str)
array(
  "blah" => 123,
)

In my opinion, this belongs as a Pull Request to Symfony. We should not partially fork or abstract the Symfony components we are using.

No one is suggesting a fork, but there is nothing wrong with not exposing an external library as a public api within our framework.

@msonnabaum: there is also no reason to add an abstraction to a component that we are using precisely so as not to maintain it. As soon as you add an abstraction like that, you need to test the abstraction so as to make sure that every implementation works the same way. Do we really want to start having YAML test cases in core?

Tagging.

It seems reasonable to do this upstream but I'm neutral on that - more interested in it happening than how.

I agree it adds some complexity on our end, but I dont think it's a reasonable request for symfony to defer to the extension if available, that's something we'd need to do ourselves.

See here: https://twitter.com/fabpot/status/303913791643594752

So this is a no-go for upstream contribution. Let's do it in core.

If it was too much trouble for Symfony to silently swap PECL vs. userspace, why do we think it would be beneficial for us to do so?

Performance?

Are we ever parsing YAML in a critical path? I thought we'd been careful to avoid that.

We're talking about adding < 20 lines of code for eeking out a bit of extra speed. It doesn't hurt to add such a tiny wrapper, does it?

Adding a wrapper will force us to have less capabilities in order to support multiple different backends. IMO Symfony's component is a good choice, and we should stick to it. The problem is that if both parser/writers have tiny differences, those tiny differences could be enought to make them non compatible between each other if we decide to do some kind advanced use of the Yaml format. Maintaining the wrapper for two backends could also become a maintainance nightmare. As Crell said, Yaml parsing should never be in a critical path, so performance doesn't really matter here, parsing Yaml is probably already fast enough for what we have to do with it.

I'd recommend to try it out before continuing to debate hypothetic meta.

There are only minor differences between the YAML 1.1 and 1.2 spec. Additionally, while Symfony implements the 1.2 spec, it does not actually support the full spec, but only a very small and basic subset of it. I studied that for some configuration system issues already.

Whether it makes sense ultimately depends on the performance of the php-yaml extension, as well as your filesystem's performance. The exact point where it will get interesting is when reading/parsing YAML files through the extension would be faster than a cache lookup. If you think about it, that's not too far stretched, so within the range of possibilities.

If anyone wants to do some benchmarking here's some code to use the PECL extension.

Interesting making this patch has discovered that we have some invalid yaml in the default system.maintenance.yml file message value. PECL issues a PHP warning... symfony doesn't... both end up just assuming it's a string.

In running the Configuration test group there is not a lot of difference in timings but this is to be expected as config is cached in the database.

However if I make the following change to the CoreBundle remove the config cache...

     $container
-      ->register('config.storage', 'Drupal\Core\Config\CachedStorage')
-      ->addArgument(new Reference('config.cachedstorage.storage'))
-      ->addArgument(new Reference('cache.config'));
+      ->register('config.storage', 'Drupal\Core\Config\FileStorage')
+      ->addArgument(config_get_config_directory(CONFIG_ACTIVE_DIRECTORY));

Symfony takes: 1 min 41 sec
PECL takes: 1 min 37 sec
With config cache (PECL): 1 min 38 sec
With config cache (Symfony): 1 min 42 sec

I would say that these results are close enough to be almost the same :) so there is no dramatic speed up here.

Note that the YAML files written by Symfony and PECL are quite different but I have not found any issues switching after the files have been written... i.e. the parsers can read each others files.

PECL looks like this:

---
name: Site-Install
mail: admin@example.com
slogan: ""
page:
  403: ""
  404: ""
  front: node
admin_compact_mode: "0"
weight_select_max: "100"
...

Symfony looks like this:

name: Site-Install
mail: admin@example.com
slogan: ''
page:
  403: ''
  404: ''
  front: node
admin_compact_mode: '0'
weight_select_max: '100'

3 to 5 seconds improvements on a complete test suite is not insignificant, but is not a real performance improvement considering this hurts only non critical paths. After reading a bit more what looks like both parsers in term of API, it's basically only read() and dump() methods, it would be easy to maintain such wrapper, so I don't really care about doing it or not, I think both solution (having a wrapper or not having a wrapper) suits well to the need.

Rather than the test suite I'd probably go for profiling a page with lots of configuration objects on a cache miss.

When a site has a completely empty cache (drush cc all / memcache restart etc.), it won't be able to serve any pages at all until particular configuration files are parsed and cached (along with lots of other cached stuff like plugins, theme registry etc.).

If this process is sufficiently slow then you can end up hitting max clients in Apache and similar limits as requests keep coming in but can't be finished. YAML parsing is unlikely to be the worst candidate here (/me looks at plugin discovery sideways), but where there's genuine performance improvement to be had in those cases we should take them when we can get them. That code tends to be shocking for performance because "well it's cached anyway isn't it" - and it's precisely when you have empty caches that your site might fall over and have trouble getting back up again.

Another place it may or may not make a difference is the installer where we have to install default config for a lot of modules all in one go - remember that some distributions might have 150 modules + a load of default config specific to the profile.

Awesome, Alex!

1. The PECL's warning about system.maintenance.yml is appropriate - a string value containing spaces has to be enclosed in quotes.
2. YAML does not make a difference between double-quotes and single-quotes. (There's also no parsing for magic values in double-quotes.) Thus, that difference is irrelevant.
3. What's more interesting is that the PECL implementation unconditionally starts a document stream, even though there is no meta data. Perhaps there's another hidden setting for that? In any case, Symfony's Yaml parser is capable of dealing with the YAML document stream delimiter, so this is perfectly fine. (However, it is not able to deal with actual meta data.)
4. A diff of 5 seconds for the Configuration system test group alone is pretty huge, given that most of the contained tests are DUTB tests (which are using Cache\MemoryBackend, which in turn means that the majority of tests are operating on readily-available native PHP arrays for the majority of [consecutive] config object reads).
5. As mentioned previously, if this makes any sense, then only when skipping the cache backend entirely and reading all data straight out of .yml files on disk. I (rather strongly) believe that this is the only performance test case of interest. If the numbers will stay positive, then the PECL extension becomes a viable alternative.
6. That said, the current numbers may be "compromised" by Cache\DatabaseBackend. Advanced caching setups may be hard to replicate and profile though (e.g., APC, Memcached, etc).

@#22 Agree cache rebuild scenario are the worst, but I don't think that file parsing will ever do the difference. Things such as menu rebuild, features rebuild, views cache rebuild are so much trouble for both CPU time and memory consuption that I don't think parsing 150 Yaml files will be significant among all that.

If we use YAML for .info files, this'll also get run on the modules page - since that gets an uncached list of modules and metadata from their info files every time it's hit. Again what percentage of time that actually is I'm not sure (we'd need to profile the patch to find out).

See also #1851234: Slow yaml parsing slows down tests which load/save many config files (e.g. views), can probably close that one as a duplicate.

Rerolling for info file change (so we can use the PECL wrapper for info file parsing too). Also opened #1942432: Use extension_loaded instead of function_exists to detect PHP extensions.

Wondering if this should be a service. Could use a factory which also allows to override it using settings(), similar to e.g. cache/keyValue/queue.

Also, \Drupal\Core\DependencyInjection\YamlFileLoader should now be considered as well.

One great thing about using a service would be that the determination of the available extensions can be done on compiletime.

An interdiff of 27 proved impossible so attached is an interdiff based on my local commits.

Patch attached

• Uses the Drupal Yaml component in the DIC's YamlFileLoader
• cleans up the Yaml component - coding standards, removes constructor from the interface and simplifies where possible
• Does a couple of micro-optimisations in order to only instantiate a Yaml object as few times as possible

On my laptop a minimal profile install is 3 seconds quicker using the PECL extension.

#30: 1920902-pecl-yaml-29.patch queued for re-testing.

Some of our YAML couldn't be read by the PECL parser and the layout module had an interesting use of config's Filestorage...

Patch attached fixes this.

Nice clean-up. We use this to read the services.yml files on which services are defined, so that's not really an option I guess, as this is as low-level as it gets.

So +1 on this. I know that @alexpott is also working with @jthorson to install the extension on our testbots and there are some promising results.

Did run the Node test group locally and got 5m24 vs 6min19 and my results are relatively stable (second run was only a few seconds apart). So this looks like a nice improvement.

Combined with the #2006434: [meta] Speed up web tests, it should even get faster, although the improvements can't simply be added of course, as that other patch removes a ton of yaml parsing that this is making faster. But it should still be save a bit per test run.

As I mentioned in #9, if we want to go this way, we need to add full tests of YAML parsing.

Also did a full test run, with a concurrency of 8, is 22m10 without this patch and 17m18 with. That's a nice improvement.

Failed to apply due to new use Symfony\Component\HttpFoundation\RedirectResponse; line added to common.inc within the patch context. Reroll below.

Shouldn't this get also a requirements entry telling what engine is used for YAML parsing?

I'm not sure that's necessary. Since this is controlled by the availability of a PHP extension, I imagine one could just look at the phpinfo page to see if the yaml extension is available.

[#2027461: Make the YAML parser a service

Reroll fail. Hmmm ... I wonder if this core/lib/Drupal/Component/Yaml directory is important? :D

Just another re-roll, not addressing the fail in #43. Disregard. Duplicate declaration.

Ugh.

No longer works after todays static YAML changes.

PHP Fatal error: Call to undefined function theme() in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc on line 941

Fun!

My patch defind the $yaml property as a static array to cache parsed files. You assign the Yaml class to the same and then access it in a non-static way.

That threw an E_STRICT very, very early. That goes into the error handler, that tries to do something with watchdog, accesses the module handler, that doesn't exist and it throws an exception. That exception is caught by the installer and then the installer tries to display it. However, theme.inc has not yet been loaded, so it dies with that Fatal Error.

Admittedly, my reroll was just a shot in the dark based on where the patch wasn't applying, as opposed to being based on any understanding of the code itself. ;)

Some testbot results:

One of our 'fast' OSUOSL testbots (659) without libYaml:
- 58,149 pass(es) in 1 hour 8 min.

One of our 'slow' OSUOSL testbots (654) without libYaml:
- 58,052 pass(es) in 1 hour 7 min.

One of our 'slow' OSUOSL testbots (dev) with libYaml:
- 56,102 pass(es), 5 fail(s), and 4 exception(s) in 58 min 1 sec.

Looks like about a 15% improvement in the overall testing time.

Results for the libYAML dev bot can be found at https://qa.staging.devdrupal.org/pifr/test/558838

libYAML testbot errors:

Drupal\filter\Tests\FilterAdminTest:
Illegal offset type in isset or empty in ckeditor.admin.inc line 36: template_preprocess_ckeditor_settings_toolbar() (3 times)

Drupal\system\Tests\Common\ParseInfoFileUnitTest:

yaml_parse(): scanning error encountered during parsing: mapping values are not allowed in this context (line 3, column 30)

yaml_parse('simple_string: A simple string
version: "VERSION"
double_colon: dummyClassName::
')
Drupal\Component\Yaml\Pecl->parse('simple_string: A simple string
version: "VERSION"
double_colon: dummyClassName::
')
Drupal\Component\Yaml\Yaml->parse('simple_string: A simple string
version: "VERSION"
double_colon: dummyClassName::
')
drupal_parse_info_file('core/modules/system/tests/common_test_info.txt')
Drupal\system\Tests\Common\ParseInfoFileUnitTest->testParseInfoFile()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('96', 'Drupal\system\Tests\Common\ParseInfoFileUnitTest')

^^^ in Pecl.php line 32 - Drupal\Component\Yaml\Pecl->parse()

Simple string value was parsed correctly.	ParseInfoFileUnitTest.php line 29	Drupal\system\Tests\Common\ParseInfoFileUnitTest->testParseInfoFile()	
Constant value was parsed correctly.	ParseInfoFileUnitTest.php line 30	Drupal\system\Tests\Common\ParseInfoFileUnitTest->testParseInfoFile()	
Value containing double-colon was parsed correctly.	ParseInfoFileUnitTest.php line 31	Drupal\system\Tests\Common\ParseInfoFileUnitTest->testParseInfoFile()	

Drupal\views\Tests\Handler\AreaEntityTest:

The rendered entity appears in the footer of the view.	AreaEntityTest.php line 99	Drupal\views\Tests\Handler\AreaEntityTest->testEntityArea()	
The rendered entity appeared in the right view mode.	AreaEntityTest.php line 100	Drupal\views\Tests\Handler\AreaEntityTest->testEntityArea()

Re-roll.

This should fix the failures.

However, these look like automatically written files, which indicates bugs in the symfony yaml dumper? Which again means, we need test coverage for them.

Also, WTH is entity_id: '!1' ?! ;)

D-D-Doublepost!

Confirmed that this passes on a testbot with libYAML installed, in 1 hour 15 seconds. Testing has shown a savings of 6-10 minutes over a testbot without libYAML.

Given #36 and #9 I'm not sure this is worth as Damien is correct will be have to assert everything we expect from a YAML parser.

I've abstracted the Yaml changes as this definitely need to be done... #2031597: Weird YAML data

55: 1920902-pecl-yaml-55.patch queued for re-testing.

Once this is done, we could do much better for testing than using libyaml, much better indeed. YAML parsing will be slow, of course, faster in C, but slow still. We could write a little PHP file which contains two functions: yaml_parse and yaml_emit. yaml_emit would just call the relevant Symfony functionality but yaml_parse would wrap it in apc_store / apc_fetch . A very handy key would be md5($input) (the pox on core's hash policy, this is not core). Then preload this wrapper with Apache php_value auto_prepend_file and Bob's your uncle: no more parsing, even across test runs.

Let's see what we can do here.

looks good #67

1. +++ b/core/lib/Drupal/Component/Yaml/Yaml.php
   @@ -0,0 +1,71 @@
   +    static $plugin;
   

   Why a static variable, and not an instance/class property?

2. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -142,51 +131,32 @@ public function rename($name, $new_name) {
   +      $this->yaml = new Yaml;
   

   Use parenthesis.

Spoke to chx on IRC, it's very late for him now :) so rerolling with those changes on his behalf.

Also did a cleanup pass of the docs.

Sorry to change back from the RTBC, but there were a few of these things that had to be fixed up :) I guess it was tested and works well though. For me the patch is functionally great.

OK, so files are attached but not here on the thread? :( wat?

Let's try these uploads once again.

interdiff looks good for me
back to RTBC

the last patch is green, so... back to rtbc per #73.

1. +++ b/core/lib/Drupal/Component/Yaml/Pecl.php
   @@ -0,0 +1,42 @@
   +    return yaml_parse($input);
   

   I wonder whether yaml_parse_file() would yield an additional performance improvement?

   (not loading the raw document into memory?)

2. +++ b/core/lib/Drupal/Component/Yaml/Pecl.php
   @@ -0,0 +1,42 @@
   +    return yaml_emit($value);
   

   Shouldn't we specify an UTF-8 encoding + LN linebreaks here?

   http://php.net/manual/en/function.yaml-emit.php

3. +++ b/core/lib/Drupal/Component/Yaml/Symfony.php
   @@ -0,0 +1,82 @@
   +  public function dump($value) {
   +    // The level where you switch to inline YAML is set to PHP_INT_MAX to ensure
   +    // this does not occur.
   +    return $this->getDumper()->dump($value, PHP_INT_MAX);
   +  }
   
   +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -142,51 +131,34 @@ public function rename($name, $new_name) {
      /**
       * Implements Drupal\Core\Config\StorageInterface::encode().
   -   *
   -   * @throws Symfony\Component\Yaml\Exception\DumpException
       */
      public function encode($data) {
        // The level where you switch to inline YAML is set to PHP_INT_MAX to ensure
        // this does not occur. Also set the exceptionOnInvalidType parameter to
        // TRUE, so exceptions are thrown for an invalid data type.
   -    return $this->getDumper()->dump($data, PHP_INT_MAX, 0, TRUE);
   +    return $this->getYaml()->dump($data, PHP_INT_MAX, 0, TRUE);
      }
   

   The additional arguments no longer exist on Yaml::dump().

   PHP_INT_MAX was moved, but why not ,0,TRUE ?

   Additionally, isn't the consuming code (+ tests?) catching some of the Symfony\Yaml exceptions and needs to be updated accordingly?

4. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php
   @@ -224,16 +224,6 @@ public function testDataTypes() {
   -    try {
   -      $config->set('stream', fopen(__FILE__, 'r'))->save();
   -      $this->fail('No Exception thrown upon saving invalid data type.');
   -    }
   -    catch (\Exception $e) {
   -      $this->pass(format_string('%class thrown upon saving invalid data type.', array(
   -        '%class' => get_class($e),
   -      )));
   -    }
   

   Hm, not really keen on removing tests.

   Does this mean that we're not throwing exceptions on invalid data types anymore?

   Can we retain some level of error handling?

Using the word "plugin" here for the yaml implementation is very confusing considering they aren't plugins. Maybe change it to implementation?

g

I wonder whether yaml_parse_file() would yield an additional performance improvement?
(not loading the raw document into memory?)

I strongly suspect you'd gain very little -- after all yaml_parse_file needs to read the file. And read #61 on a significant test spedeup that using yaml_parse opens up.

Can we retain some level of error handling?

Nope. Talk to the YAML extension people, there's nothing yaml_emit gives you.

Made those changes to the YAML implementation class, and replaced the word plugin with implementation.

Added back the addition parameters to YAML dump, it was only really the last one that was doing anything before. Not sure what to do about losing that test. I think that is ok tbh. If we make the changes to the testbots to use Pecl YAML, and it doesn't throw exceptions, not too sure what we can do about that.

Thanks! Let's move forward here.

I think it's worth adding the hook_requirements() as suggested in #40, although getting the wording right might be tricky. Could be a follow-up, but we have similar for PECL image uploads iirc.

Are there any differences between the output of Symfony and PECL for the files that get written?

Also at least major. Cache misses == runtime.

That sounds like a needs review/needs work. Splitting the difference. :)

@catch see #20 for differences between the output of PECL and Symfony's YAML dumpers.

I personally think we should do the hook_requirements stuff in a follow up. Doc related patches often end with perpetual bikeshedding etc...

It would be good to get this change in.

Opened #2157447: Add hook_requirements() for PECL YAML parser.

Would be nice if there was a formatting option for single vs. double quotes for strings. But if there isn't then I don't think that should hold this up since both are equally fine.

Back to RTBC.

There is not an option for either to configure the formatting of string quotes, I'm afraid.

Yeah the only real references I could find to the string quotes being different were comments in this issue :p

Do we feel that #9 and https://twitter.com/fabpot/status/303913791643594752 can be ignored? I'm not sure that we've even run the full test suite with the PECL library.

If we're going to have YAML in the critical path (i.e. the production configuration storage), then we need to address the performance issue. I disagree with #8 that the handful of extra lines of code are a problem, that's a lot less code than we've added to solve other performance issues.

I'd expect the test suite to find issues and I'm not convinced we need explicit YAML testing beyond that (at least for initial commit), but yeah I don't know if anyone's actually run that and we shouldn't commit until that's been done and confirmed.

I'd be OK with not having it swappable/using PECL at all if we did https://drupal.org/project/config_db in core but that'd be a big change.

Instead of

Proposed resolution

Add a YAML class that decides whether to use the extension or Symfony.

wouldn't it be possible to default to one (or the other) and allow the Drupal user in configuration to choose which one to use?

It seems, from reading the comments above, that the code to automatically select which is the best one to use is problematical, and indeed the question of which is the best YAML parser may change over time.

So, isn't it possible to have in Drupal configuration something like:

Select YAML parser:

• PECL (default. Slower, but guaranteed to always work, since supports YAML 1.2)
• Symfony (faster, where performance really matters, but only supports YAML 1.1)
• Mega-Woosh (fastest of all, but only supports subset of YAML 1.1)
• Awesome-Advanced (supports YAML 1.9, but deadslow)

wouldn't it be possible to default to one (or the other) and allow the Drupal user in configuration to choose which one to use?

Not if you are storing those settings from the list in the configuration system. As you will need the config system to parse stuff to get the setting before choosing the parser :)

This could be an option in settings.php at best imo. That could be a followup.

Yeah that has to be settings.php (which you can use to switch out services, or just D7 style switch using Settings). It's a good idea.

Indeed. Do you agree we should open a follow up to discuss that? But yes, I think just a $setting would work just fine here. As well as being pretty much the only option :)

We would probably just want to add that logic to the new Yaml factory class provided by this patch.

Given how little it takes to make this settings.php overrideable and given that I already have an idea of why we would want to do that, I am adding it.

After seeing another seemingly random setting name/value being introduced here, I just filed #2160705: settings.php setting names are inconsistent (and undocumented)

ok, thanks for opening that. It's a worthy discussion to have :) We can add this to the list in that issue.

On that note, I think this issue is done.

97: 1920902_97.patch queued for re-testing.

Neither alexpott or myself are very hot on this because of #12 (this is always going to be code we have to maintain, because Symfony has rejected this approach), and also (for my part) because we really don't need more environment-specific reasons that testbot might fail but localhost might not.

However, catch seemed to think this would be important for run-time performance. Assigning accordingly.

The amount of code to maintain here is minimal - entire patch is 13kb.

The main argument against this is that we could get into a situation where the YAML dumped/parsed between the PECL and Symfony parsers isn't compatible, since that'd mean corruption. I don't think this is at all likely, although I'd feel more comfortable if we had a test that dumped with one, read back with the other, then vice-versa.

YAML was originally rejected as a format in large part due to performance (see https://groups.drupal.org/node/167584, which includes xhprof results showing the PECL parser to at least ten times faster than Symfony's - 44ms vs. 2882ms to parse 500 files).

On real sites, we can expect hundreds of YAML files between fields, instances, display modes, views etc. - and those will all need to be parsed from the filesystem when the cache is empty. The difference between (for example) 30ms and 2 seconds in the critical path to serve any page at all in such a situation is significant - especially given all the other file system scanning and parsing we have with plugins etc., so I don't think there's an option but to go ahead with this.

However, leaving this open for a day or two more so we can discuss how feasible a parsing test is (obviously it'll only run on a system with the PECL extension installed, but presumably we'll want to add that to the bots for performance anyway), and in case Alex/webchick have any more comments.

I think that extra test is a great idea and would go a long way toward making me more comfortable with this.

97: 1920902_97.patch queued for re-testing.

97: 1920902_97.patch queued for re-testing.

Reroll and adds requested tests.

+++ b/core/tests/Drupal/Tests/Component/Yaml/YamlTest.php
@@ -0,0 +1,90 @@
+
+/**
+ * Tests the Drupal\Component\Uuid\Uuid class.
+ *
+ * @group Drupal
+ * @group YAML
+ */
+class UuidTest extends UnitTestCase {
+

Yeah, I don't think so :)

Larowlan--
Will fix in morning

Fixed test class name and and the ability to skip the test if the Pecl Yaml extension is not installed as it obviously is not on the testbots.

Also added a test for YAML node anchors which has highlighted a difference between Symfony and Pecl. Pecl does not support dots in anchor names.

jquery.ui:
  version: &jquery_ui 1.10.2

jquery.ui.accordion:
  version: *jquery_ui

Works on both.

jquery.ui:
  version: &jquery.ui 1.10.2

jquery.ui.accordion:
  version: *jquery.ui

Only works in Symfony

This is very relevant for #1996238: Replace hook_library_info() by *.libraries.yml file which is making extension use of node anchors with dots in.

1. +++ b/core/lib/Drupal/Component/Yaml/Yaml.php
   @@ -0,0 +1,85 @@
   +    elseif (function_exists('yaml_emit')) {
   

   Here we are using function_exists but in the test we are using extension_loaded(), we should make these uniform.

2. +++ b/core/tests/Drupal/Tests/Component/Yaml/YamlTest.php
   @@ -0,0 +1,127 @@
   +      $this->markTestSkipped(
   +           'The PECL Yaml extension is not available.'
   +      );
   

   This could just be one one line.

Pretty sure I rerolled it right according to instructions above

...remember to hide old patches/files when uploading new ones ;)

Re-implemented as Drupal\Core\Serialization\Yaml. - SerializationInterface concept courtesy of #2190723: Add a KeyValueStore\FileStorage to replace e.g. ConfigStorage

The previous location in Component wasn't really appropriate, because we're adjusting both Yaml implementations to follow Drupal's coding standards, and the Yaml "factory" has a dependency on Settings, which happens to live in Component right now, but is misplaced there.

The new Yaml factory ensures that the implementation is only instantiated once per request. In particular for the PECL implementation, this means we're overriding PHP INI settings only once.

My goal for D8 is to make the following default serialization formats easily available to the entire application:

Drupal\Core\Serialization\Json (moved from Drupal\Component\Utility\Json)
Drupal\Core\Serialization\PhpSerialize
Drupal\Core\Serialization\Yaml
Drupal\Core\Serialization\Xml

SerializationInterface uses static methods to make these simple encoding/decoding helper functions available to any code. The classes can still be instantiated and can also be registered/injected as services, which I actually did in #2190723: Add a KeyValueStore\FileStorage to replace e.g. ConfigStorage, but intentionally left out of this patch here.

I manually tested both implementations locally, and Drupal installs fine with both. (PECL is tremendously faster, of course.)

As promised: Green. :-)

The PHPUnit test to cross-test the PECL vs. Symfony implementations is still contained. Some other classes are merely simplified, since they no longer need to instantiate an object.

Moving forward here would certainly help especially core developers who need to re-install and run rebuild.php very often.

I am fine with switching this to a static implementation. As the instances were being created before solely to call the encode/decode method. No state is really involved.

1. +++ b/core/lib/Drupal/Core/Serialization/SerializationInterface.php
   @@ -0,0 +1,45 @@
   +  public static function getFileExtension();
   

   Does this really need to be a part of the interface? The file format and the actual serialization does not seem that useful to couple together IMO.

2. +++ b/core/tests/Drupal/Tests/Component/Yaml/YamlTest.php
   @@ -0,0 +1,126 @@
   +class YamlTest extends UnitTestCase {
   

   As you're in this file anyway, we could add some @covers

We will need to check about your proposed change of moving to Drupal\Core\Serialization\Json. As this will break any existing code.

I'm happy to remove the getFileExtension() method from this patch, if you feel strongly about that, but we're going to need it later on anyway.

Corrected unit test location + added @covers.

I rerolled against latest head.

Now that Yaml::decode returns a static, it was conflicting somehow with the parsedInfos array in the InfoParser class. xdebug was showing two parsedInfos arrays defined, so I rolled back the static modifiers in InfoParser.php and changed it back to the $this->parsedInfos methodology as it existed before #2188661: Extension System, Part II: ExtensionDiscovery was applied.

I ran /admin/modules on a fresh cache without pecl yaml installed and everything worked great. I then installed pecl yaml, and everything still worked. I did some comparisons before and after the patch on a freshly rebuilt cache and have attached the xhprof summary.

The interdiff is super chundery, cause I couldnt figure out how to get an interdiff on a reroll with merge conflicts, so I just diffed the diffs. hopefully its reviewable.

I'm not able to confirm the hiccups mentioned in #120.

Therefore, attaching a re-roll of #119 (merged 8.x).

I had a mergefail

-          static::$this->parsedInfos[$filename] = Yaml::decode(file_get_contents($filename));
+          static::$parsedInfos[$filename] = Yaml::decode(file_get_contents($filename));

none of the hiccups in 120 were valid.

Thanks for this, btw. This will really help the YAML performance, especially when developing/site building.

Thanks for confirming, @Mixologic.

Any chance to move forward here? I think this patch is ready to be committed.

Created #2208609: Move Json utility class into Drupal\Component\Serialization

I'm sorry but when you basically rewrite a patch. You start the review process again :)

Created #2208633: Add a Yaml class to Drupal\Component\Serialization

This patch can not be tested properly on testbot until the pecl yaml extension is installed. I think this patch will pass even though there are PHPUnit failures since the test will be skipped.

Also I think that we need to ensure all YAML files we create in core are equivalently decode-able...

Currently they are not...

There were 3 failures:

1) Drupal\Tests\Core\Serialization\YamlTest::testYamlFilesInCore with data set #295 ('/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/language/config/language.mappings.yml')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'no' => 'nb'

@@ @@
     'zh-chs' => 'zh-hans'
+    '' => 'nb'
 )

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Serialization/YamlTest.php:140
/usr/local/php5-5.3.25-20130615-024255/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.3.25-20130615-024255/lib/php/PHPUnit/TextUI/Command.php:129

2) Drupal\Tests\Core\Serialization\YamlTest::testYamlFilesInCore with data set #493 ('/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/system/config/system.date_format.html_year.yml')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     'pattern' => Array (
-        'php' => 'Y'
-        'intl' => 'Y'
+        'php' => true
+        'intl' => true
     )
 )

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Serialization/YamlTest.php:140
/usr/local/php5-5.3.25-20130615-024255/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.3.25-20130615-024255/lib/php/PHPUnit/TextUI/Command.php:129

3) Drupal\Tests\Core\Serialization\YamlTest::testYamlFilesInCore with data set #883 ('/Volumes/devdisk/dev/sites/drupal8alt.dev/core/profiles/standard/config/editor.editor.full_html.yml')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
                             4 => 'Subscript'
-                            5 => '-'
+                            5 => Array (...)
                             6 => 'RemoveFormat'
                         )
                     )
                     1 => Array (...)
                     2 => Array (...)
                     3 => Array (...)
                     4 => Array (...)
                     5 => Array (...)
                 )
             )
         )
         'plugins' => Array (...)
     )
     'image_upload' => Array (...)
     'status' => true
     'langcode' => 'en'
 )

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Serialization/YamlTest.php:140
/usr/local/php5-5.3.25-20130615-024255/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.3.25-20130615-024255/lib/php/PHPUnit/TextUI/Command.php:129

I still agree with @DamZ in #9 this is exceptionally risky. The above evidence shows Symfony Yaml parser would write a value Y without quotes and PECL would interpret this as bool. This means switching YAML parsers on an installed site looks impossible.

Here is proof of our problems...

With the patch in #129 we get the following fail:

1) Drupal\Tests\Core\Serialization\YamlTest::testEncodeDecode with data set #1 (array('bar', 'schnitzel', array('nope', 'thanks'), array('if', 'ask', 'nicely'), array(array('would', 'Y')), 123, false, 1, false, array(1, false), array(10), array('123456')), 'Drupal\\Core\\Serialization\\YamlPecl', 'Drupal\\Core\\Serialization\\YamlSymfony')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
             'still' => 'would'
-            'be' => 'Y'
+            'be' => true
         )
     )
     'how_many_times' => 123
     'should_i_ask' => false
     0 => 1
     1 => false
     2 => Array (...)
     3 => Array (...)
     4 => Array (...)
 )

Yuk. So the difference between pecl and symfony is boolean mangling and some other subtle problems.

It seems to me that the main value of having the pecl module is of primary benefit to dev environments (module building, site building, cold caches) - environments where the 'critical path' is redefined.

Given that, I think that this should only be enabled by a settings.php configuration, and don't believe that there should be automatic use of the Pecl yaml if the extension is loaded (imagine some shared host that just happens to have it installed etc.).

If a user has configured settings.php to use the YamlPecl, but doesn't actually have the extension loaded, we should throw an exception.

Additionally, there is the issue of configuration exports. I suggest that YamlPecl extends the YamlSymphony class and override the decode method, and avoid yaml_emit altogether. The majority of the performance gain happens on the decode side, and that would eliminate the possiblity of conflicting exports going from dev to production, or packaged as part of a distribution.

Posted some benchmarks when running tests in #2006434-22: [meta] Speed up web tests.

yaml pecl and symfony pecl are different enough (pecl = 1.1 yaml, symfony = subset of 1.2 yaml) That mixing the two totally fails.

I just ran a fresh installation with each parser, enabled every module, exported the configs, stripped out the uuid fields, and ran a diff.

Needless to say, it's a mess. Im completely unable to import a pecl-yaml config into an install running symfony (it throws a Drupal\Core\Config\UnsupportedDataTypeConfigException: Invalid data type in config editor.editor.basic_html: Unable to parse at line 8 (near " items:").)

I wish there were a C/C++ version of libyaml that supported 1.2 and that *also* had a php wrapper around it.

As slow as the symfony php parser is, I think our best option here is to improve that upstream as best as we can. Either that or put in some very, very stark warnings that if you use pecl yaml, your entire infrastructure has to support it, from dev to prod, and even then stuff is likely to fail.

My earlier idea of just using pecl to decode won't work as whatever it misinterprets on the input side would still be incorrect on the output using the Symfony encoder.

I really wish we could somehow use this, as it's so much faster. :(

Just for completeness, merged/re-rolled this against HEAD.

This patch can not be tested properly on testbot until the pecl yaml extension is installed.

Let's test anyway.

I think this is really a non-starter unless we have a PHP implementation and PECL that support the same spec and pass the same tests.

I think #1907170: Very simple config files can't be read is related.

[removed duplicate comment]

re-roll

Wondering if we can use http://php.net/manual/en/yaml.callbacks.parse.php to help with the boolean differences between 1.1 and 1.2 - bool is a tag.

1.2: http://www.yaml.org/spec/1.2/spec.html
1.1: http://yaml.org/type/bool.html

Yes! we can use callbacks - have the Y/y/N/n fixed.
Also filed #2400769: Duplicate label key in views data schema (patch included here)
Next issue is the 'some/sort/of/hybrid/key/0' yaml keys in migrate yml files which throw an exception and the
- - entry in core/profiles/standard/config/install/editor.editor.full_html.yml

Plugging on

Also found #2400771: Duplicate entry for user views data schema and #2400773: Remove empty text.data_types.schema.yml

Fixes the - - editor issue.
Fixes some intentional 'invalid' yml
Fixes re-roll muck-up which resulted in fatals during test setup.
Includes three issues opened as indicated by #143 and #145

Should be just the migrate file issues left (at least for YamlTest phpunit tests)

Actually, the migrate issues aren't the use of the single quotes, but the use of @ in the values.
Checking for that in the yaml spec.

It looks like we're using @ for aliases?
Yaml Spec says we should be using * and &? http://www.yaml.org/spec/1.2/spec.html#id2786196 - and for 1.1 http://www.yaml.org/spec/1.1/#id899912 so does that mean the '@' is a Drupalism?
If so....we might just be able to make this work.
If anyone can point me to where the '@' stuff came from/is processed/etc that'd be great

nvm, single quoting the lines with '@' works, now for the test fails.

quotes '@stuff' in migrate yml files and fixes test fails - but also starts using Drupal\Core\Serialization\Yaml over Drupal\Component\Serialization\Yaml (except in the context of components)

This one swaps it in

Ok ready for real reviews here - also what's involved in getting the yaml pecl component on testbots - to see how much of a performance boost this is for testing?

@ is from Symfony docs but I'm not sure it is canon http://symfony.com/doc/current/components/dependency_injection/advanced....

Just a few nitpicks:

1. +++ b/core/lib/Drupal/Component/Serialization/Yaml.php
   @@ -7,36 +7,56 @@
   +  protected static $instance;
   

   Can we call this 'serializer' rather than 'instance'?

2. +++ b/core/lib/Drupal/Component/Serialization/YamlPecl.php
   @@ -0,0 +1,113 @@
   +    throw new InvalidDataTypeException($message, $severity);
   

   Are we losing some useful information here?

+++ b/core/lib/Drupal/Component/Serialization/YamlPecl.php
@@ -88,4 +89,25 @@ public static function applyBooleanCallbacks($value, $tag, $flags) {
+  /**
+   * Applies sequence after parsing to ignore 1.1 issues.
+   *
+   * @param mixed $value
+   *   Value from YAML file.
+   * @param string $tag
+   *   Tag that triggered the callback.
+   * @param int $flags
+   *   Scalar entity style flags.
+   *
+   * @return array|string
+   *   Yaml 1.1 decodes - - as an array containg a NULL, in 1.2 it should be the
+   *   string '-'.
+   */
+  public static function applySequenceCallbacks($value, $tag, $flags) {
+    if ($value == [NULL]) {
+      return '-';
+    }
+    return $value;
+  }

Personally I think we should make editor module use something other than '-' to represent a spacer plugin and drop this hunk altogether

#155 1: fixed, 2: no, not really

Also fixed #156 by quoting the -

1. diff --git a/core/includes/common.inc b/core/includes/common.inc
   index 11f8a9d..6ea9bf9 100644
   --- a/core/includes/common.inc
   +++ b/core/includes/common.inc
   @@ -9,30 +9,22 @@
     */
    
    use Drupal\Component\Serialization\Json;
   -use Drupal\Component\Serialization\Yaml;
   -use Drupal\Component\Serialization\Exception\InvalidDataTypeException;
    use Drupal\Component\Utility\Bytes;
    use Drupal\Component\Utility\Crypt;
    use Drupal\Component\Utility\Html;
   -use Drupal\Component\Utility\Number;
    use Drupal\Component\Utility\SafeMarkup;
    use Drupal\Component\Utility\SortArray;
    use Drupal\Component\Utility\String;
   -use Drupal\Component\Utility\Tags;
    use Drupal\Component\Utility\UrlHelper;
    use Drupal\Core\Cache\Cache;
   -use Drupal\Core\Language\LanguageInterface;
   +use Drupal\Core\PhpStorage\PhpStorageFactory;
    use Drupal\Core\Site\Settings;
    use Drupal\Core\Url;
    use Symfony\Component\HttpFoundation\Response;
    use Symfony\Component\HttpFoundation\Request;
   -use Drupal\Core\PhpStorage\PhpStorageFactory;
    use Drupal\Component\Utility\NestedArray;
   -use Drupal\Core\Datetime\DrupalDateTime;
   -use Drupal\Core\Routing\GeneratorNotInitializedException;
    use Drupal\Core\Template\Attribute;
    use Drupal\Core\Render\Element;
   -use Drupal\Core\Session\AnonymousUserSession;
    
    /**
     * @defgroup php_wrappers PHP wrapper functions
   

   out of scope?

2. +++ b/core/lib/Drupal/Component/Serialization/YamlPecl.php
   @@ -0,0 +1,91 @@
   +    set_error_handler(array(__CLASS__, 'errorHandler'));
   ...
   +    restore_error_handler();
   ...
   +  public static function errorHandler($severity, $message, $file, $line) {
   +    restore_error_handler();
   

   I'm not familiar with error handlers, but its odd that you have to restore error handlers eon every call to them? Maybe document why this is needed.

3. +++ b/core/lib/Drupal/Core/Serialization/Yaml.php
   @@ -0,0 +1,48 @@
   +    }
   +    $settings = Settings::getInstance();
   +    // If there is a settings.php override, use that.
   +    if ($settings && ($class = $settings->get('yaml_parser_class'))) {
   +      static::$serializer = new $class();
   +    }
   +    // If the PECL YAML extension installed, use that.
   +    elseif (extension_loaded('yaml')) {
   +      static::$serializer = new YamlPecl();
   +    }
   +    else {
   +      // Otherwise, fallback to the Symfony implementation.
   +      static::$serializer = new YamlSymfony();
   +    }
   +  }
   +
   

   Good old times, when settings used to be a component and we did not needed all that shizzle all over the place.

4. +++ b/core/modules/system/src/Tests/Extension/InfoParserUnitTest.php
   @@ -81,7 +81,8 @@ public function testInfoParser() {
   -    $this->assertEqual($info_values['double_colon'], 'dummyClassName::', 'Value containing double-colon was parsed correctly.', 'System');
   +    debug($info_values);
   

   leftover debug :)

5. +++ b/core/modules/user/config/schema/user.views.schema.yml
   @@ -68,17 +68,13 @@ views_field_user:
   -  type: views_field
   +  type: views_field_user
   

   This change is fine, but in general it is super confusing that its not named views.field.user as well.

6. +++ b/core/modules/views/config/schema/views.data_types.schema.yml
   @@ -365,7 +365,6 @@ views_argument:
        title:
          type: label
   -      label: 'Title'
   

   mh? Doesn't that cause a regression in config translation?

1 will fix
2 yeah this was from an earlier patch, seems pecl/yaml doesn't throw a catchable exception?
3 again from earlier patch, is there a better way now?
4 will fix
5, 6 they are duplicated in HEAD see linked issues - basically those keys are given a value twice in the yaml

Fixes 158.1 and 4
Updates fix for user.views.schema.yml as per linked issue
Waiting for answers to questions about 2 and 3.
6 is fixed in HEAD

Note https://github.com/symfony/symfony/issues/13209

Adds comment for 158.2 - related issue is #1247666: Replace @function calls with try/catch ErrorException blocks
Need more info for 158.3 - @dawehner can you elaborate?

Need more info for 158.3 - @dawehner can you elaborate?

Well, when Settings used to be part of \Drupal\Components you would not need to override various classes for settings specific overrides. All those overrides are sad, honestly.

Ah, so that means nothing I can do about it here?
Sorry if I'm being dense.

So ready for reviews again - would love to get this in.

Are we sure these are the only differences?

There's a new test that checks all yml files in core. If you have pecl tank installed it's passing. We need it on the bots before this goes in.

There's more to yaml than meets the eye... erm, our config files ;)

Take this example:

$data = [
  'key' => 'This is a string with
multiple
newlines',
];

ini_set('yaml.output_width', -1);
ini_set('yaml.output_indent', 2);
$yaml_pecl = yaml_emit($data, YAML_UTF8_ENCODING, YAML_LN_BREAK);
var_dump($yaml_pecl);

$yaml_symfony = Yaml::dump($data, PHP_INT_MAX, 2, TRUE);
var_dump($yaml_symfony);

Gives this:

string(62) "---
key: |-
  This is a string with
  multiple
  newlines
...
"
string(49) "key: "This is a string with\nmultiple\nnewlines"
"

It is compatible, both can parse both versions, but the default (?) output is different. Which will be very annoying if you have yaml extension on your servers but not locally, we use the yaml format to diff staging/active...

Another example, from https://github.com/symfony/symfony/pull/11976

$expected = array('float' => (float) 1);
$yaml = Yaml::dump($expected);
var_dump($yaml);

$expected = array('float' => (float) 1);
$yaml = yaml_emit($expected, YAML_UTF8_ENCODING, YAML_LN_BREAK);
var_dump($yaml);

Gives me:

string(17) "float: !!float 1
"

string(24) "---
float: 1.000000
...
"

Again, both parse both versions correctly as a (float) 1, but the yaml is different. What are we going to do about this?

I would propose that we don't do anything :) If you have a difference between your prod, dev and staging environments etc, then you have bigger problems. The 'works on my machine' debate etc.
Functionally they are the same right? It's just the human readable output and version control you're referring to right?

Staging and production I agree, but having differences between local development systems and servers as far as php versions and extensions go is very common.

Depends whether you consider a configuration import that never shuts up about having changes to import functional or not ;) I would, and our tests think so too (As they verify that there are no changes after doing an import).

Also, consider a user case where a contrib module adds a revert functionality, by comparing the config you have with the default configuration somewhere in a module. Not all contrib developers will have the same environment. Extending on that, this will obviously also affect patches with default configuration for core and contrib.

What if we decoded then encoded the YAML for the diffs rather than using the raw file contents? That'd also take into account hand-edited files.

Also, you can use settings.php to nominate to use symfony/yaml everywhere if it is an issue between your local dev/staging etc.

Note we can also utilize emit callbacks :
http://php.net/manual/en/yaml.callbacks.emit.php

Edit: no we can't - those are for objects only

pinged @jthorson to request a test bot with pecl/yaml.
Note to self: remind him at the weekend

Rerolling. One of the major conflicts was from #1907170: Very simple config files can't be read which, apart from changes to \Drupal\Component\Serialization\Yaml, introduced a new YamlTest.php too. Instead of merging, I just changed the new YamlTest.php in the patch to YamlPeclTest.php as it really relies on the PECL extension to work (the test is skipped if the extension isn't present).

Reroll.

Also, cleaned up some docs and updated and cleaned up some code.

Ran a quick install comparison. Super scientific and all.

Without:
d8install 52.39s user 1.13s system 85% cpu 1:02.37 total
d8install 53.45s user 1.49s system 85% cpu 1:04.57 total
d8install 51.69s user 1.01s system 84% cpu 1:02.25 total

With:
d8install 34.34s user 1.50s system 79% cpu 44.807 total
d8install 34.78s user 1.02s system 78% cpu 45.818 total
d8install 33.51s user 1.27s system 78% cpu 44.128 total

50s.. I assume you either have xdebug and/or xhprof/uprofiler enabled?

Try without, On my system, the installer is almost twice as fast without xdebug.

Also, might be worth testing it with the file cache patch applied, my I guess is that the difference will be much smaller, because we only parse each yml file once instead of up to 40 times.

xdebug is turned off. I do more then just drush si in that script though.

numbers without xdebug.

Without:
d8install 25.37s user 0.81s system 74% cpu 35.328 total
d8install 25.93s user 1.01s system 74% cpu 36.291 total
d8install 25.73s user 0.94s system 74% cpu 35.689 total

With:
d8install 15.92s user 0.84s system 68% cpu 24.595 total
d8install 18.43s user 0.84s system 67% cpu 28.353 total
d8install 16.81s user 0.88s system 66% cpu 26.582 total

So about 8 seconds which is actually a 33% decrease instead of 28% so a little bit better.

No more instantiation. Better unit tests. Fix a couple yml files that didn't parse right. Details in interdiff.

I'm pretty happy with where things are at this point.

Turns out this found a bug in DrupalStandards listener. Since this breaks the phpunit test runner when the yaml extension is not installed this is sort of blocked until that is resolved.

Also, fix another yaml error. Based on the number of yaml changes in the patch, it seems Symfony's implementation has some quirks...

reroll around conflict

Chx updated the migrate documentation so the changes in the patch are documented.
https://www.drupal.org/node/2135307/revisions/view/7082283/8298081
https://www.drupal.org/node/2135345/revisions/view/6924553/8298083

String -> Safemarkup chuck collision.

chuck collision. no changes.

Actually a little more then that. I ran unit tests before uploading this but got distracted and forgot to look at the results. #2470928: Versions in core.libraries.yml are not always parsed as strings fixes a lot of our YAML. However it adds some tests around dates that are tied to symfony's implementation of tag resolution.

Unfortunately this causes our "all of core parses the same in PECL and Symfony" test to fail. This is a bit annoying because we've built a test for the behavior of our yaml parser into a library test which is really "unit" testing.

I've been spent a good portion of this afternoon scouring both the 1.1 and 1.2 version of the spec and this line keeps getting in the way "Tag resolution is specific to the application". This rule specifically applies to complex types like timestamps which are discussed but not actively defined by the spec. I tried to turn on yaml.decode_timestamp which sounds like it would act the same way symfony's implementation does but it actually resolves more then really should including parsing quoted and "non-specific" tagged nodes.

I've opened up a bug report here: https://bugs.php.net/bug.php?id=69465 Its possible that the bug is the other way and the bare tag should be parsed without that ini setting... or they are both bugs... I'm not really sure. We can probably work around this similar to the way to what we did for bools but I haven't gotten to it.

Fix invalid YAML from #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name
Remove library test because I can't figure out what its testing other then YAML parses like YAML. There is no specific functionality its testing in the Library code.

Clearly there's a bit of a "bug" in PECL's date parsing though. :-/

reroll for another chunk collision

1. +++ b/core/lib/Drupal/Component/Serialization/Yaml.php
   @@ -52,4 +44,21 @@ public static function getFileExtension() {
   +   * Determines the optimal implementation to use for encoding and parsing Yaml.
   

   s/Yaml/YAML

2. +++ b/core/lib/Drupal/Component/Serialization/Yaml.php
   @@ -52,4 +44,21 @@ public static function getFileExtension() {
   +      // If the PECL YAML extension installed, use that.
   

   I'd suggest replacing the comment by: "Use the PECL YAML extension if it is available."

3. +++ b/core/lib/Drupal/Component/Serialization/Yaml.php
   @@ -52,4 +44,21 @@ public static function getFileExtension() {
   +  }
   

   The closing brace for the class must have an empty line before it.

4. +++ b/core/lib/Drupal/Core/Serialization/Yaml.php
   @@ -0,0 +1,33 @@
   +  protected static function getSerializer(){
   

   There should be a space after the closing parenthesis.

5. +++ b/core/lib/Drupal/Core/Serialization/Yaml.php
   @@ -0,0 +1,33 @@
   +    // If there is a settings.php override, use that.
   

   Maybe change the comment for something similar to "Use a settings.php override if any"

6. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlBaseTest.php
   @@ -0,0 +1,101 @@
   + * Provides test cases for Yaml tests so we can assert certain things parse the
   

   s/Yaml/YAML

   Also "certain things" could probably use some more clarification.

7. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlBaseTest.php
   @@ -0,0 +1,101 @@
   +   * @return array
   

   Missing the @return comment on new line.

8. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlBaseTest.php
   @@ -0,0 +1,101 @@
   +          'nicely'
   

   Missing a comma.

9. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlBaseTest.php
   @@ -0,0 +1,101 @@
   +  public function providerDecodeTests() {
   

   This function is missing doc comment.

10. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlBaseTest.php
    @@ -0,0 +1,101 @@
    +      // Null files.
    

    Should this be s/Null/NULL?

11. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlBaseTest.php
    @@ -0,0 +1,101 @@
    +      // Node Anchors.
    

    I don't think 'Anchors' needs a capital 'A'.

12. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlBaseTest.php
    @@ -0,0 +1,101 @@
    +  public function providerBoolTest() {
    

    This function is missing doc comment.

13. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,86 @@
    + * @coversDefaultClass \Drupal\Component\Serialization\YamlPecl
    

    @group and @coversDefaultClass sections should be separated by a blank line.

14. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,86 @@
    + * @requires extension yaml
    

    The @coversDefaultClass and @requires sections should be separated by a blank line.

15. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,86 @@
    +   * @dataProvider providerEncodeDecodeTests
    

    @covers and @dataProvider sections should be separated by a blank line.

16. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,86 @@
    +   * @dataProvider providerDecodeTests
    

    Ditto.

17. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,86 @@
    +   * @dataProvider providerBoolTest
    +   * @param string $string
    

    Ditto.

18. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,86 @@
    +   *   The expected return
    

    Missing a full stop.

19. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,86 @@
    +   * @expectedException \Drupal\Component\Serialization\Exception\InvalidDataTypeException
    

    @covers and @expectedException sections should be separated by a blank line.

20. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,86 @@
    +}
    

    The closing brace for the class must have an empty line before it.

21. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
    @@ -0,0 +1,70 @@
    + * @coversDefaultClass \Drupal\Component\Serialization\YamlSymfony
    

    @group and @coversDefaultClass sections should be separated by a blank line.

22. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
    @@ -0,0 +1,70 @@
    + * @requires extension yaml
    

    @coversDefaultClass and @requires sections should be separated by a blank line.

23. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
    @@ -0,0 +1,70 @@
    +   * @dataProvider providerEncodeDecodeTests
    

    Ditto.

24. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
    @@ -0,0 +1,70 @@
    +   * @dataProvider providerDecodeTests
    

    Ditto.

25. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
    @@ -0,0 +1,70 @@
    +   * @expectedException \Drupal\Component\Serialization\Exception\InvalidDataTypeException
    

    Ditto.

26. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
    @@ -0,0 +1,70 @@
    +}
    

    The closing brace for the class must have an empty line before it.

27. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +   * in our YAML that might not be decoded correctly in on of our
    

    s/on/one

28. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +   * @TODO this should exist as an integration test no as part of our unit tests.
    

    s/TODO/todo

    s/this/This

    s/no/not

    => With those changes, the todo should be wrapped at 80 chars.

29. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +   * @dataProvider providerYamlFilesInCore
    

    @requires and @dataProvider should be separated by a blank line.

30. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +   * Data provider that lists all YAML files core.
    

    s/files core/files in core?

31. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +   * @return array
    

    There must be a blank line between the comment and the @return tag.

    The @return tag is missing the return comment on new line.

32. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +}
    

    The closing brace for the class must have an empty line before it.

33. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +class YamlStub extends Yaml {
    

    class is missing its doc comment.

34. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +  public static function getSerializer() {
    

    The function is missing its doc comment.

35. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +}
    

    The closing brace for the class must have an empty line before it.

36. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +class YamlParserProxy implements SerializationInterface {
    

    The class is missing its doc comment.

37. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,117 @@ class YamlTest extends UnitTestCase {
    +  public static function setMock($mock) {
    ...
    +  public static function encode($data) {
    ...
    +  public static function decode($raw) {
    ...
    +  public static function getFileExtension() {
    

    The functions are missing their doc comment.

Thanks, that's a really extensive review.

13-25,29) separated annotations
Spent 10min scrounging google, issues queues, and docs and couldn't find a standard on it and I've never done this in other tests I've written.

@return comments - I'd rather remove them then duplicate the method documentation.

33,34,36,37) This is standard for stub objects in tests.

#$%^ simpletest....

Renamed abstract base test to work around weird "@group" test in simpletest discovery.