Problem/Motivation

We should be defaulting to the PECL Yaml component if it is available as opposed to Symfony's because:
it is a performance improvement for reads (decoding)
and is YAML complient.

(Also, see #249)

We had originally talked about this way back in the beginning but it had been forgotten about.

Pecl YAML Profile

Proposed resolution

Add a YAML class that decides whether to use the extension or Symfony. There's no point in using plugins and plugins are too heavy for this -- it's not extensible.

Also, Created Symfony issue for their invalid parsing: https://github.com/symfony/symfony/issues/16234

They seem to be aligning themselves with PECL as well:
http://symfony.com/blog/new-in-symfony-3-1-yaml-deprecations

API changes

Config save no longer throws exceptions if the config object contains an object or resource. While we could make Symfony do this, yaml_emity() doesn't do this so we can't make this a requirement.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task|Bug because it is a performance improvement
Issue priority Major, because it saves actually something
Unfrozen changes No
Prioritized changes Yes, performance improvements
Disruption None

Major, no disruption, performance improvement so allowed.

CommentFileSizeAuthor
#325 add_a_drupal_yaml-1920902-325.interdiff.txt1.72 KBneclimdul
#325 add_a_drupal_yaml-1920902-325.patch62.15 KBneclimdul
#323 add_a_drupal_yaml-1920902-321.patch60.42 KBneclimdul
#321 add_a_drupal_yaml-1920902-321.patch60.39 KBneclimdul
#321 add_a_drupal_yaml-1920902-321.interdiff.txt9.14 KBneclimdul
#314 add_a_drupal_yaml-1920902-312.patch54.84 KBneclimdul
#312 add_a_drupal_yaml-1920902-312.interdiff.txt16.64 KBneclimdul
#312 add_a_drupal_yaml-1920902-312.patch54.81 KBneclimdul
#308 add_a_drupal_yaml-1920902-308.patch38.47 KBneclimdul
#308 1920902.mergdiff.txt491 bytesneclimdul
#305 add_a_drupal_yaml-1920902-305.patch38.42 KBneclimdul
#291 add_a_drupal_yaml-1920902-291.patch39.08 KBneclimdul
#291 add_a_drupal_yaml-1920902-291.interdiff.txt838 bytesneclimdul
#289 add_a_drupal_yaml-1920902-289.patch39.09 KBneclimdul
#289 add_a_drupal_yaml-1920902-289.interdiff.txt1.08 KBneclimdul
#288 add_a_drupal_yaml-1920902-286.patch39.4 KBneclimdul
#286 add_a_drupal_yaml-1920902-286.patch0 bytesneclimdul
#279 add_a_drupal_yaml-1920902-279.patch39.17 KBneclimdul
#277 interdiff-1920902-277.txt3.33 KBneclimdul
#277 add_a_drupal_yaml-1920902-277.patch39.06 KBneclimdul
#274 add_a_drupal_yaml-1920902-274.patch39.73 KBneclimdul
#273 add_a_drupal_yaml-1920902-273.patch47.47 KBneclimdul
#272 add_a_drupal_yaml-1920902-272.patch39.68 KBneclimdul
#268 add_a_drupal_yaml-1920902-268.patch40.27 KBneclimdul
#268 add_a_drupal_yaml-1920902-268.interdiff.txt599 bytesneclimdul
#265 add_a_drupal_yaml-1920902-265.patch39.68 KBneclimdul
#263 add_a_drupal_yaml-1920902-263.patch0 bytesneclimdul
#260 inerdiff.1920902.259.260.txt1.32 KBYesCT
#260 add_a_drupal_yaml-1920902-260.patch40.27 KBYesCT
#259 interdiff.1920902.255.259.txt12.81 KBYesCT
#259 add_a_drupal_yaml-1920902-259.patch39.92 KBYesCT
#255 1920902-255.interdiff.txt474 bytesneclimdul
#255 add_a_drupal_yaml-1920902-255.patch39.12 KBneclimdul
#251 add_a_drupal_yaml-1920902-251.patch39.11 KBneclimdul
#251 interdiff-1920902-251.txt2.06 KBneclimdul
#247 add_a_drupal_yaml-1920902-247.patch39.32 KBneclimdul
#240 add_a_drupal_yaml-1920902-240.patch39.3 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,737 pass(es). View
#237 1920902-237.interdiff.txt9.21 KBneclimdul
#237 add_a_drupal_yaml-1920902-237.patch39.33 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch add_a_drupal_yaml-1920902-237.patch. Unable to apply patch. See the log in the details link for more information. View
#233 add_a_drupal_yaml-1920902-233.patch30.12 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,654 pass(es). View
#229 add_a_drupal_yaml-1920902-229.patch30.15 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,526 pass(es), 11 fail(s), and 2 exception(s). View
#229 1920902-229.interdiff.txt1.89 KBneclimdul
#225 drupal_yaml-1920902-225.patch28.98 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,461 pass(es). View
#221 yaml-pecl-1920902-2.221.patch28.76 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch yaml-pecl-1920902-2.221.patch. Unable to apply patch. See the log in the details link for more information. View
#219 1920902-219.interdiff.txt3.76 KBneclimdul
#219 add_a_drupal_yaml-1920902-219.patch35.37 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,463 pass(es). View
#217 add_a_drupal_yaml-1920902-217.patch40.35 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,475 pass(es), 3 fail(s), and 0 exception(s). View
#217 1920902-217.interdiff.txt2.43 KBneclimdul
#213 add_a_drupal_yaml-1920902-212.patch35.93 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,824 pass(es). View
#212 add_a_drupal_yaml-1920902-212.patch0 bytesneclimdul
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,811 pass(es). View
#211 pecl-YAML-profile.png40.45 KBneclimdul
#210 add_a_drupal_yaml-1920902-210.patch36.06 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,861 pass(es). View
#209 1920902-209.interdiff.txt590 bytesneclimdul
#209 add_a_drupal_yaml-1920902-209.patch36.01 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,824 pass(es). View
#207 1920902-207.interdiff.txt611 bytesneclimdul
#207 add_a_drupal_yaml-1920902-207.patch36.01 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#205 add_a_drupal_yaml-1920902-205.patch36.02 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#203 add_a_drupal_yaml-1920902-203.patch36.54 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#198 add_a_drupal_yaml-1920902-198.patch36.49 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#196 1920902-interdiff.txt5.78 KBneclimdul
#196 add_a_drupal_yaml-1920902-196.patch36.49 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#194 add_a_drupal_yaml-1920902-194.patch36.4 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,299 pass(es). View
#193 interdiff.txt5.97 KBneclimdul
#193 add_a_drupal_yaml-1920902-193.patch38.82 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,924 pass(es). View
#190 add_a_drupal_yaml-1920902-190.patch34.05 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,491 pass(es), 546 fail(s), and 311 exception(s). View
#189 add_a_drupal_yaml-1920902-189.patch34.45 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,478 pass(es). View
#188 add_a_drupal_yaml-1920902-188.patch34.45 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,430 pass(es). View
#187 add_a_drupal_yaml-1920902-187.patch34.37 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,184 pass(es). View
#187 add_a_drupal_yaml-1920902-187.interdiff.txt1.33 KBneclimdul
#186 add_a_drupal_yaml-1920902-186.interdiff.txt21.2 KBneclimdul
#186 add_a_drupal_yaml-1920902-186.patch33.77 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,790 pass(es). View
#182 1920902-182.interdiff.txt3.99 KBneclimdul
#182 add_a_drupal_yaml-1920902-182.patch25.38 KBneclimdul
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,124 pass(es). View
#179 yaml-pecl.179.patch26.53 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch yaml-pecl.179.patch. Unable to apply patch. See the log in the details link for more information. View
#162 yaml-pecl.162.patch26.21 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch yaml-pecl.162.patch. Unable to apply patch. See the log in the details link for more information. View
#162 interdiff.txt1.15 KBlarowlan
#160 yaml-pecl.160.patch26.31 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,350 pass(es). View
#160 interdiff.txt2.68 KBlarowlan
#157 yaml-pecl.157.patch28.38 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,312 pass(es). View
#157 interdiff.txt4.61 KBlarowlan
#152 yaml-pecl.152.patch28.47 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,311 pass(es). View
#152 interdiff.txt1.74 KBlarowlan
#151 Screenshot 2015-01-02 11.53.05.png73.91 KBlarowlan
#151 yaml-pecl.151.patch26.73 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,313 pass(es). View
#151 interdiff.txt14.84 KBlarowlan
#146 yaml-pecl.146.patch15.88 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,313 pass(es), 2 fail(s), and 0 exception(s). View
#146 interdiff.txt3.38 KBlarowlan
#143 yaml-pecl.142.patch14.12 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View
#143 interdiff.txt2.13 KBlarowlan
#141 yaml-pecl.141.patch12.65 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View
#134 yaml.pecl_.134.patch10.78 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#133 symfonyvspecl.diff305.49 KBMixologic
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch symfonyvspecl.diff. Unable to apply patch. See the log in the details link for more information. View
#129 127-129-interdiff.txt523 bytesalexpott
#129 1920902.129.patch23.9 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,832 pass(es). View
#127 1920902.127.patch23.9 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,802 pass(es). View
#127 121-127-interdiff.txt1.32 KBalexpott
#121 yaml.pecl_.121.patch22.95 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es). View
#120 Screen Shot 2014-03-01 at 11.37.44 AM.png41.33 KBMixologic
#120 interdiff.txt2.79 KBMixologic
#120 yaml.pecl_.120.patch23.46 KBMixologic
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es). View
#119 interdiff.txt3.19 KBsun
#119 yaml.pecl_.119.patch22.97 KBsun
PASSED: [[SimpleTest]]: [MySQL] 64,723 pass(es). View
#116 interdiff.txt26.98 KBsun
#116 yaml.pecl_.116.patch22.6 KBsun
PASSED: [[SimpleTest]]: [MySQL] 64,589 pass(es). View
#113 interdiff.txt1.12 KBrcaracaus
#113 1920902.113.patch6.62 KBrcaracaus
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#111 1920902.111.patch15.7 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,750 pass(es). View
#111 107-111-interdiff.txt1.68 KBalexpott
#107 yml-extension.107.patch14.85 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#107 interdiff.txt2.18 KBlarowlan
#97 interdiff.txt685 byteschx
#97 1920902_97.patch13.71 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1920902_97.patch. Unable to apply patch. See the log in the details link for more information. View
#95 1920902_94.patch13.67 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#95 interdiff.txt884 byteschx
#79 interdiff-1920902-79.txt2.99 KBdamiankloip
#79 1920902-79.patch13.46 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,207 pass(es). View
#20 1920902-pecl-yaml-20.patch12.14 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,194 pass(es). View
#27 1920902_27-pecl-yaml.patch13.17 KBcweagans
PASSED: [[SimpleTest]]: [MySQL] 53,159 pass(es). View
#30 1920902-pecl-yaml-29.patch14.99 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,552 pass(es). View
#30 27-29-pseudo-interdiff.txt7.36 KBalexpott
#33 1920902-pecl-yaml-33.patch19.68 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,976 pass(es). View
#33 30-33-interdiff.txt3.77 KBalexpott
#38 1920902-pecl-yaml-38.patch14.25 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#43 1920902-pecl-yaml-43.patch19.71 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] 58,544 pass(es), 1 fail(s), and 0 exception(s). View
#48 1920902-pecl-yaml-47.patch19.48 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#46 1920902-pecl-yaml-46.patch19.75 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php. View
#51 1920902-pecl-yaml-51-interdiff.txt1.11 KBBerdir
#51 1920902-pecl-yaml-51.patch19.74 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,052 pass(es). View
#55 1920902-pecl-yaml-55-interdiff.txt1.5 KBBerdir
#55 1920902-pecl-yaml-55.patch21.24 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1920902-pecl-yaml-55.patch. Unable to apply patch. See the log in the details link for more information. View
#65 1920902_65.patch11.78 KBchx
FAILED: [[SimpleTest]]: [MySQL] 57,954 pass(es), 1 fail(s), and 0 exception(s). View
#67 1920902_67.patch12.61 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,922 pass(es). View
#67 interdiff.txt706 byteschx
1920902-70.patch13.34 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 59,087 pass(es), 2 fail(s), and 1 exception(s). View
interdiff-1920902-70.txt5.3 KBdamiankloip
#72 1920902-70.patch13.34 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,958 pass(es). View
#72 interdiff-1920902-70.txt5.3 KBdamiankloip
Membership dollars fund testing for the Drupal project. Drupal Association Learn more

Comments

cweagans’s picture

Issue tags: +Novice

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.

msonnabaum’s picture

Title: Default to PECL Yaml component if it is available » Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available
Issue tags: -Novice

Yes, lets.

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

sun’s picture

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.

msonnabaum’s picture

msonnabaum’s picture

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,
)
msonnabaum’s picture

Damien Tournoud’s picture

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

msonnabaum’s picture

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

Damien Tournoud’s picture

@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?

catch’s picture

Issue tags: +Performance

Tagging.

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

msonnabaum’s picture

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.

cweagans’s picture

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

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

Crell’s picture

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?

cweagans’s picture

Performance?

Crell’s picture

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

heyrocker’s picture

We are not, even if #1793074: Convert .info files to YAML gets in I don't think that will make any real difference.

cweagans’s picture

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?

pounard’s picture

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.

sun’s picture

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.

alexpott’s picture

Status: Active » Needs review
FileSize
12.14 KB
PASSED: [[SimpleTest]]: [MySQL] 52,194 pass(es). View

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'
pounard’s picture

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.

catch’s picture

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.

sun’s picture

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).
pounard’s picture

@#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.

catch’s picture

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).

Berdir’s picture

cweagans’s picture

FileSize
13.17 KB
PASSED: [[SimpleTest]]: [MySQL] 53,159 pass(es). View

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.

Berdir’s picture

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.

dawehner’s picture

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

alexpott’s picture

FileSize
14.99 KB
PASSED: [[SimpleTest]]: [MySQL] 55,552 pass(es). View
7.36 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Performance

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

alexpott’s picture

FileSize
19.68 KB
PASSED: [[SimpleTest]]: [MySQL] 56,976 pass(es). View
3.77 KB

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.

Berdir’s picture

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.

Berdir’s picture

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.

Damien Tournoud’s picture

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

Berdir’s picture

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

jthorson’s picture

FileSize
14.25 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

claudiu.cristea’s picture

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

cweagans’s picture

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.

chx’s picture

jthorson’s picture

FileSize
19.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,544 pass(es), 1 fail(s), and 0 exception(s). View

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

Berdir’s picture

Status: Needs work » Needs review
jthorson’s picture

Status: Needs work » Needs review
FileSize
19.75 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php. View

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

jthorson’s picture

Status: Needs work » Needs review
FileSize
19.48 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Ugh.

jthorson’s picture

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
19.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,052 pass(es). View

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.

jthorson’s picture

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. ;)

jthorson’s picture

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

jthorson’s picture

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()
Berdir’s picture

FileSize
1.5 KB
21.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1920902-pecl-yaml-55.patch. Unable to apply patch. See the log in the details link for more information. View

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' ?! ;)

Berdir’s picture

D-D-Doublepost!

jthorson’s picture

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.

alexpott’s picture

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

jibran’s picture

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

chx’s picture

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.

chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
chx’s picture

Assigned: Unassigned » chx

Let's see what we can do here.

chx’s picture

Status: Needs work » Needs review
FileSize
11.78 KB
FAILED: [[SimpleTest]]: [MySQL] 57,954 pass(es), 1 fail(s), and 0 exception(s). View
chx’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
12.61 KB
PASSED: [[SimpleTest]]: [MySQL] 57,922 pass(es). View
706 bytes
podarok’s picture

Status: Needs review » Reviewed & tested by the community

looks good #67

damiankloip’s picture

  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.

damiankloip’s picture

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.

damiankloip’s picture

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

damiankloip’s picture

FileSize
13.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,958 pass(es). View
5.3 KB

Let's try these uploads once again.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

interdiff looks good for me
back to RTBC

herom’s picture

Status: Needs work » Reviewed & tested by the community

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

sun’s picture

  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?

msonnabaum’s picture

Status: Reviewed & tested by the community » Needs work

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

chx’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
13.46 KB
PASSED: [[SimpleTest]]: [MySQL] 59,207 pass(es). View
2.99 KB

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.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Let's move forward here.

catch’s picture

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?

catch’s picture

Priority: Normal » Major

Also at least major. Cache misses == runtime.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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

alexpott’s picture

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

damiankloip’s picture

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.

catch’s picture

Status: Needs review » Reviewed & tested by the community

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.

damiankloip’s picture

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

catch’s picture

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

alexpott’s picture

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.

catch’s picture

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.

drupalshrek’s picture

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)
damiankloip’s picture

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.

catch’s picture

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.

damiankloip’s picture

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.

chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
884 bytes
13.67 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

chx’s picture

Status: Needs work » Needs review
FileSize
13.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1920902_97.patch. Unable to apply patch. See the log in the details link for more information. View
685 bytes
sun’s picture

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

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

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

webchick’s picture

Assigned: Unassigned » catch

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.

catch’s picture

Assigned: catch » Unassigned

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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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

jibran’s picture

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

larowlan’s picture

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

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.18 KB
14.85 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Reroll and adds requested tests.

Berdir’s picture

+++ 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’s picture

Larowlan--
Will fix in morning

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
15.7 KB
PASSED: [[SimpleTest]]: [MySQL] 63,750 pass(es). View

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.

damiankloip’s picture

  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.

rcaracaus’s picture

FileSize
6.62 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
1.12 KB

Pretty sure I rerolled it right according to instructions above

klonos’s picture

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

sun’s picture

Component: configuration system » base system
Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
22.6 KB
PASSED: [[SimpleTest]]: [MySQL] 64,589 pass(es). View
26.98 KB

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.)

sun’s picture

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.

damiankloip’s picture

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.

sun’s picture

FileSize
22.97 KB
PASSED: [[SimpleTest]]: [MySQL] 64,723 pass(es). View
3.19 KB

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.

Mixologic’s picture

FileSize
23.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es). View
2.79 KB
41.33 KB

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.

sun’s picture

FileSize
22.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es). View

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

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

Mixologic’s picture

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.

sun’s picture

Thanks for confirming, @Mixologic.

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

sun’s picture

damiankloip’s picture

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

sun’s picture

alexpott’s picture

FileSize
1.32 KB
23.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,802 pass(es). View

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
alexpott’s picture

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.

alexpott’s picture

FileSize
23.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,832 pass(es). View
523 bytes

Here is proof of our problems...

alexpott’s picture

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 (...)
 )
Mixologic’s picture

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.

Berdir’s picture

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

Mixologic’s picture

Status: Needs review » Needs work
FileSize
305.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch symfonyvspecl.diff. Unable to apply patch. See the log in the details link for more information. View

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. :(

sun’s picture

Assigned: sun » Unassigned
FileSize
10.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

jibran’s picture

Status: Needs work » Needs review

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

Let's test anyway.

pwolanin’s picture

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.

YesCT’s picture

YesCT’s picture

[removed duplicate comment]

larowlan’s picture

Status: Needs work » Needs review
FileSize
12.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.13 KB
14.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View

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

larowlan’s picture

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.38 KB
15.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,313 pass(es), 2 fail(s), and 0 exception(s). View

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)

larowlan’s picture

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.

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
14.84 KB
26.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,313 pass(es). View
73.91 KB

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)

larowlan’s picture

FileSize
1.74 KB
28.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,311 pass(es). View

This one swaps it in

larowlan’s picture

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?

cilefen’s picture

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

kim.pepper’s picture

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?

larowlan’s picture

+++ 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

larowlan’s picture

FileSize
4.61 KB
28.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,312 pass(es). View

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

Also fixed #156 by quoting the -

dawehner’s picture

  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?

larowlan’s picture

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

larowlan’s picture

FileSize
2.68 KB
26.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,350 pass(es). View

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

larowlan’s picture

larowlan’s picture

Assigned: Unassigned » dawehner
Related issues: +#1247666: Replace @function calls with try/catch ErrorException blocks
FileSize
1.15 KB
26.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch yaml-pecl.162.patch. Unable to apply patch. See the log in the details link for more information. View

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?

dawehner’s picture

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.

larowlan’s picture

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

larowlan’s picture

Assigned: dawehner » Unassigned
larowlan’s picture

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

dawehner’s picture

Are we sure these are the only differences?

larowlan’s picture

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.

Berdir’s picture

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?

larowlan’s picture

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?

Berdir’s picture

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.

catch’s picture

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.

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

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

jibran queued 162: yaml-pecl.162.patch for re-testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
26.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch yaml-pecl.179.patch. Unable to apply patch. See the log in the details link for more information. View

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).

anavarre queued 179: yaml-pecl.179.patch for re-testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
25.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,124 pass(es). View
3.99 KB

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

Berdir’s picture

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.

neclimdul’s picture

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

neclimdul’s picture

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.

neclimdul’s picture

FileSize
33.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,790 pass(es). View
21.2 KB

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.

neclimdul’s picture

Related issues: +#2453627: Fix DrupalStandardsListener for non-TestCase objects
FileSize
1.33 KB
34.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,184 pass(es). View

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...

neclimdul’s picture

FileSize
34.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,430 pass(es). View

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

neclimdul’s picture

FileSize
34.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,478 pass(es). View

String -> Safemarkup chuck collision.

neclimdul’s picture

FileSize
34.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,491 pass(es), 546 fail(s), and 311 exception(s). View

chuck collision. no changes.

neclimdul’s picture

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.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
38.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,924 pass(es). View
5.97 KB

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. :-/

neclimdul’s picture

FileSize
36.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,299 pass(es). View

reroll for another chunk collision

anavarre’s picture

Status: Needs review » Needs work
  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.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
36.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
5.78 KB

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.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
36.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

#$%^ simpletest....

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

Status: Needs work » Needs review
anavarre’s picture

Thanks!

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.

IIRC, Coder sniffs will alert about that + I've seen it in core files quite frequently so assumed it's a best practice. Random example:

tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php

261   /**
262    * @covers ::save
263    * @covers ::doSave
264    *
265    * @param \Drupal\Core\Entity\EntityInterface $entity
266    *
267    * @return \Drupal\Core\Entity\EntityInterface
268    *
269    * @depends testCreate
270    */
271   public function testSaveInsert(EntityInterface $entity) {

Would love to have confirmation though.

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

Didn't know, thanks.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
36.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Reroll

Also I don't think I was really clear in my previous rushed response. By standard I meant common or de-facto rather then a documented standard. Documentation standards are considerably more lax in testing classes.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
36.02 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Re-roll. Now that we have the new bots, will poke people to get yaml installed so the included tests aren't skipped and run.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
36.01 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
611 bytes

thing

neclimdul’s picture

Status: Needs work » Needs review
FileSize
36.01 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,824 pass(es). View
590 bytes
neclimdul’s picture

FileSize
36.06 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,861 pass(es). View

Figures I'd get a chunk conflict within hours of re-rolling this.

neclimdul’s picture

Issue summary: View changes
FileSize
40.45 KB

Shouldn't be any surprise but the cold cache profile is very good.

Pecl YAML Profile

neclimdul’s picture

FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,811 pass(es). View

Reroll around the great migrate move of 2015

neclimdul’s picture

FileSize
35.93 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,824 pass(es). View

the real patch.

joelpittet’s picture

neclimdul’s picture

Issue summary: View changes

Touching up beta eval. The blob of table tags can be hard to parse sometimes.

neclimdul’s picture

Issue summary: View changes
neclimdul’s picture

FileSize
2.43 KB
40.35 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,475 pass(es), 3 fail(s), and 0 exception(s). View

I guess another re-roll...

neclimdul’s picture

Status: Needs work » Needs review
FileSize
35.37 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,463 pass(es). View
3.76 KB

didn't merge #2490388: InfoParserUnitTest web test classes mislabeled as unit tests correctly and accidentally re-added the test and fixture.

neclimdul’s picture

Couldn't find the drupalci issue for PECL YAML so filed one.

larowlan’s picture

FileSize
28.76 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch yaml-pecl-1920902-2.221.patch. Unable to apply patch. See the log in the details link for more information. View

another reroll

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks pretty good for me. Not sure whether I'm feeling knowledgable enough, anyway

  1. +++ b/core/lib/Drupal/Component/Serialization/Yaml.php
    @@ -52,4 +44,22 @@ public static function getFileExtension() {
    +        static::$serializer = '\Drupal\Component\Serialization\YamlPecl';
    ...
    +        static::$serializer = '\Drupal\Component\Serialization\YamlSymfony';
    

    nitsuggestion: You could use ::class, just to make things a bit easier

  2. +++ b/core/lib/Drupal/Component/Serialization/YamlPecl.php
    @@ -0,0 +1,99 @@
    +    static $init;
    +    if (!isset($init)) {
    

    is there any benefit from using a class static?

hussainweb’s picture

Status: Needs work » Needs review
FileSize
28.98 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,461 pass(es). View

It was a straight reroll.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

neclimdul’s picture

oh my! thanks guys!

#222.2 I honestly can't remember why its written like that at this point.

Mixologic’s picture

Okay, I have good news and bad news. The good news is that with pecl yaml installed on a testbot, and no other changes, everything passes. So we can go ahead and add this to all the testbots without worrying whether it will break things:

https://dispatcher.drupalci.org/job/DCI_XML/143/testReport/
https://dispatcher.drupalci.org/job/DCI_XML/143/consoleFull

I went ahead and ran an ad-hoc test with the patch in 225, and
it did give the testbots a good speed kick. Not *tons*, but significant:

without patch: Test run duration: 27 min 58 sec
with patch: Test run duration: 25 min 23 sec

The bad news is the tests didnt pass:
https://dispatcher.drupalci.org/job/DCI_XML/144/testReport/
https://dispatcher.drupalci.org/job/DCI_XML/144/consoleFull

For those phpunit tests that failed, I have more data from the bot itself.

 <testcase name="testYamlFiles with data set #97" assertions="1" time="0.016048">
        <failure type="PHPUnit_Framework_ExpectationFailedException">Drupal\Tests\Component\Serialization\YamlTest::testYamlFiles with data set #97 ('/var/www/html/core/profiles/s...lt.yml')
/var/www/html/core/profiles/standard/config/install/core.entity_view_display.node.article.default.yml
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
             'type' =&gt; 'comment_default'
-            'weight' =&gt; 20
+            'weight' =&gt; 110
             'settings' =&gt; Array (...)
             'third_party_settings' =&gt; Array ()
         )
         'field_image' =&gt; Array (...)
         'field_tags' =&gt; Array (...)
         'links' =&gt; Array (...)
     )
     'hidden' =&gt; Array (...)
 )

/var/www/html/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php:80
</failure>
      </testcase>

and

<testcase name="testInfoParserCommonInfo" class="Drupal\Tests\Core\Extension\InfoParserUnitTest" file="/var/www/html/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php" line="147" assertions="0" time=
"0.003607">
      <error type="PHPUnit_Framework_ExceptionWrapper">Drupal\Tests\Core\Extension\InfoParserUnitTest::testInfoParserCommonInfo
Drupal\Core\Extension\InfoParserException: Unable to parse vfs://modules/fixtures/common_test.info.txt yaml_parse(): scanning error encountered during parsing: mapping values are not allowed in this context (line 7,
 column 30)

/var/www/html/core/lib/Drupal/Core/Extension/InfoParserDynamic.php:30
/var/www/html/core/lib/Drupal/Core/Extension/InfoParser.php:27
/var/www/html/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php:164
</error>
    </testcase>
neclimdul’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.89 KB
30.15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,526 pass(es), 11 fail(s), and 2 exception(s). View

Fixes for failures.

neclimdul’s picture

self review:

  1. +++ b/core/profiles/standard/config/install/core.entity_view_display.node.article.default.yml
    @@ -45,13 +45,6 @@ content:
    -  comment:
    -    label: above
    -    type: comment_default
    -    weight: 110
    -    settings:
    -      pager_id: 0
    -    third_party_settings: {  }
    

    This looks bad, its actually duplicate code and the two parsers are handling it differently. Seems someone was manually tinkering?

  2. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -152,7 +152,7 @@ public function testInfoParserCommonInfo() {
    -double_colon: dummyClassName::
    +double_colon: dummyClassName::method
    
    @@ -164,7 +164,7 @@ public function testInfoParserCommonInfo() {
    -    $this->assertEquals($info_values['double_colon'], 'dummyClassName::', 'Value containing double-colon was parsed correctly.');
    +    $this->assertEquals($info_values['double_colon'], 'dummyClassName::method', 'Value containing double-colon was parsed correctly.');
    

    colon followed by white space is creating a mapping which isn't valid in this location. either quote or have something after it.

neclimdul’s picture

Status: Needs work » Needs review

Heh... i've had about enough of your antics pifr.

neclimdul’s picture

FileSize
30.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,654 pass(es). View

clean merge reroll. use chunk conflict with #2451411: Add libraries-override to themes' *.info.yml.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - This looks good to me, but we need to discuss the disruption for contrib.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
39.33 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch add_a_drupal_yaml-1920902-237.patch. Unable to apply patch. See the log in the details link for more information. View
9.21 KB

Thanks mixologic for hacking drupalci so we can test this.

The ConfigDiffTest failure had trouble with pecl putting a "---" on the first line but that not matching the exports. String diffs versus data diffs. The diff arrays aren't convenient to work with but added a smarter(but pretty hacky) assertion wrapper that tests specific changes in the diff array.

The ConfigSimpleImportExportTest was similarly doing string matches. Converted this one to compare data though.

Standard installer test was asserting Symfony's array indenting. Both of these are valid and parse the same in both parsers but don't match the string match.

Foo:
- bar
- baz
Foo:
  - bar
  - baz
neclimdul’s picture

Status: Needs work » Needs review
FileSize
39.3 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,737 pass(es). View
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

Ticking some credit boxes @YesCT's request - I'm still not convinced this patch is a great idea for core. What might be a good idea is making it so that contrib can swap it out. Without having to swap everything out.

neclimdul’s picture

In my opinions the benefits far out weight any drawback and I've worked to make sure the tests are verbose and clear as you'll see. First, let me re-iterate some arguments for why this should be in core though.

  1. We get actual YAML validation for everything instead of accepting what works in Symfony's weird implementation. This mean testbot catches broken YAML before it gets out to sites using PECL or breaking any non-Drupal system that might be parsing out YAML but not using Symfony's implementation. Ruby, perl, Go, node, etc..
  2. We get a decrease in testbot runtime. Mixologic said in cut 2.5 minutes off test runs. This translates to decrease in testbot costs and developer time.
  3. It provides, as of the last benchmark, 14% decrease in cold cache startup for sites that can use it which is pretty huge. We've written entire subsystems for less.
  4. It has no API change, no noticeable performance impact(without pecl), cleaner code, and more test coverage.

The downside seems to be that testbot might throw errors that don't occur locally. Examples of failures that might show up in testbot:

String difference. These are the hardest. The difference are currently limited it seems to prefixing YAML with "---" and maybe some indention. They are a bit tricky to test but generally can by handled by parsing and comparing to a dataset. Since testing strings coming out of the YAML parser should be the domain of the YAML parser/emitter itself those sorts of test have a little bit of code smell anyways. If we _have_ to get the output closer it seems fair that we can address that in follow up.

Invalid YAML that worked in Symfony. I actually put some effort to make sure the error bubbles up through phpunit so its pretty clear. Edited core.services.yml to have an invalid tag, this was the output:

Exception thrown parsing /home/.../public_html/d8/core/core.services.yml:
yaml_parse(): scanning error encountered during parsing: found character that cannot start any token (line 54, column 17), context while scanning for the next token (line 54, column 17)

Something that parses differently. In my experience this is caused by Symfony using the first value of duplicates and PECL implementation using the second. The spec is weird and says there MUST not be duplicates but it also says implementation should be silent about parsing them and nothing else. Its implied it should match JSON parsing which I think makes PECL more correct but either way this is catching buggy YAML.

/home/.../public_html/d8/core/core.services.yml
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
             'gc_maxlifetime' => 200000
-            'cookie_lifetime' => 2000000
+            'cookie_lifetime' => 1000000
         )
         'twig.config' => Array (...)
         'renderer.config' => Array (...)
         'factory.keyvalue' => Array (...)
         'factory.keyvalue.expirable' => Array (...)
         'filter_protocols' => Array (...)
     )
     'services' => Array (...)
 )

Edit: clarify #4 in benefits

Fabianx’s picture

+1 to getting this in.

The best way would have been to have a YAML 1.1 compatibility mode on the Symfony parser ...

YesCT’s picture

wanted to summarize some discussion:

concern that if pecl is on a dev env, and not on prod (or the other way around)
then dev and prod sites that are otherwise the same, may differ.

we run into that same problem with, for example, people who have different php versions on dev and prod
people have been able to solve that, by writing env's down in revisionable code: like vagrant build scripts,
to make sure the environments are the same for dev and prod.

similar to the version of php, people would have to make sure it included (or did not include) the pecl yaml component.

so ensuring behavior consistent across envs is not a new problem, and has a solution.

neclimdul’s picture

Yeah, if you're developing on an environment that doesn't match your production there are any number of problems that can creep up. Just to re-iterate because this keeps coming up, this would only affect manually written YAML. Symfony correct escapes things when creating even if it allows invalid tags to be parsed.

neclimdul’s picture

neclimdul’s picture

Assigned: Unassigned » neclimdul
Related issues: +#2591827: DrupalCI should report code style issues - YAML

I'll claim trying to get this resolved in 8.0 for now.

Created Symfony issue for their invalid parsing: https://github.com/symfony/symfony/issues/16234
Created follow up to add contrib coding standards check: #2591827: DrupalCI should report code style issues - YAML

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +rc target

Discussed at length with @catch.

I think we can make this an RC target for the following reasons:

  • The APCu filecache should make reads cheap on some sites however it is extremely unlikely that shared hosting will be able to install APCu and use it securely. Therefore doing this change will help such hosting have much better performance. For the same reason it will be very useful on large multi-site installs.
  • It does help ensure that all Yaml is Yaml specification compliant.

However in order to commit this patch during RC we need to make the following changes:

  • @catch and I agreed that the performance part of this is about reading Yaml not writing. Because Yaml is our data interchange file format we should only use Symfony to write - this will reduce the risk of making this change during the RC and where production and development does not match. (great idea whoever had it - I thought I read it on this issue but I can not find it)
  • We also agreed that in order to proceed with these change we need to have at least one more test environment that is using Symfony to read yaml files so that this common environment is at least tested.
Fabianx’s picture

+1 to #249:

- I 100% agree that we use only Symfony to write
- In theory - if strict mode does not happen upstream - could we try re-reading the file with Symfony if PECL yaml parsing fails / throws an Exception?

neclimdul’s picture

Assigned: neclimdul » Unassigned
Status: Needs work » Needs review
FileSize
2.06 KB
39.11 KB

I've disagreed with not proxying the encode method in the past but lets just get this in. I'll leave the testing stuff to the testing group.

Fabianx’s picture

#251: Could you answer the question:

"- In theory - if strict mode does not happen upstream - could we try re-reading the file with Symfony if PECL yaml parsing fails / throws an Exception?"

Is that feasible - we could still alert via watchdog or an assertion or a trigger_error, but would not affect production sites.

dawehner’s picture

It would be great to put some line there explaining why we want to use the same code always for writing.

neclimdul’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
39.12 KB
474 bytes

Fix the brown bag error in the last patch.

- In theory - if strict mode does not happen upstream - could we try re-reading the file with Symfony if PECL yaml parsing fails / throws an Exception?
The "strict mode" in the Symfony issue would only affect reading. None of the fixes that have come out of this issue seem to come from export code but from manual editing. So writing and then re-reading would not really help anything.

Someone else will have to write the line for why, I don't really know why we've decided to do it because writing differences have never been a problem in this issue.

Fabianx’s picture

#255: What I mean is regardless of any upstream changes:

- Given I manually edit a file
- Given it is read correctly by Symfony
- Given it fails on pecl

- When I install the pecl extension on production
- Then I expect Drupal to gracefully fail
- And read the file via Symfony

--

e.g.

decode:

  if ($serializer instanceof YamlPecl) {
    try {
      $result = $serializer::decode(...);
    }
    catch (Exception $e) {
      $result = Symfony::decode(...);
      trigger_error('Detected parsing difference, fix file ...', E_USER_WARNING);
    }
  }
  else {
      $result = $serializer::decode(...);
  }

    return $result;

That should alleviate alexpott's concerns.

neclimdul’s picture

As noted in the summary, yaml_emit() doesn't throw exceptions so no go with at least that implementation.

Honestly, if you're going to run different environments in productions and dev and not have any testing in between there's only so much we can do.

YesCT’s picture

Assigned: Unassigned » YesCT

I'll take a stab at writing the comment, and also open the issue for the testing environment.

YesCT’s picture

but first just some nit fixes.

  1. +++ b/core/lib/Drupal/Component/Serialization/Yaml.php
    @@ -7,11 +7,8 @@
    
     namespace Drupal\Component\Serialization;
    
    -use Drupal\Component\Serialization\YamlPecl;
    -use Drupal\Component\Serialization\YamlSymfony;

    took out these unused use statements (not needed because they are in the same namespace)

  2. +++ b/core/lib/Drupal/Component/Serialization/YamlPecl.php
    @@ -0,0 +1,99 @@
    +  public static function errorHandler($severity, $message, $file, $line) {
    

    I'm guessing this was copied from somewhere, but we dont use $file and $line.

    looked at:
    ag "function errorHandler"

    for some examples and some dont have the file and line, so I think we can not have it here either.

  3. +++ b/core/lib/Drupal/Component/Serialization/YamlPecl.php
    @@ -0,0 +1,99 @@
    +    // YAML 1.1 spec dictates that 'Y', 'N', 'y' and 'n' are booleans, we want
    +    // the 1.2 behaviour so we only case 'false', 'FALSE', 'true' OR 'TRUE' as
    +    // booleans.
    

    broke this up into separate sentences, fixed grammar a bit, and used US english spelling (895 vs 23 usages).

  4. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -8,7 +8,7 @@
     use Drupal\Component\Diff\Diff;
    -use Drupal\Component\Serialization\Yaml;
    +use Drupal\Core\Serialization\Yaml;
     use Drupal\Core\Config\Entity\ConfigDependencyManager;
    

    put some changed uses in alphabetical order. (only in the lines changed by the file, so the interdiff shows changes in order, but the patch should be still all in scope)

  5. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
    @@ -0,0 +1,87 @@
    +   * @param string $string
    +   *   String value for the yaml boolean.
    +   * @param string|bool $expected
    +   *   The expected return
    

    expected can be a string or bool? but string is always a string?

    Assert same would check the type also.

    Just want to make sure that we want to do it like we are.

  6. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,104 @@ class YamlTest extends UnitTestCase {
    +   * This tests is a little bit slow but it tests that we don't have any bugs
    +   * in our YAML that might not be decoded correctly in on of our
    +   * implementations.
    

    fixed some typos.

  7. +++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
    @@ -20,44 +24,104 @@ class YamlTest extends UnitTestCase {
    +   * @todo This should exist as an integration test not part of our unit tests.
    +   *
    

    added an issue for this @todo. #2597730: Test should be an integration test and not a unit test for all YAML files being decoded in the same way with Symfony and PECL.

YesCT’s picture

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

says optimal, but doesn't list why it is better.

changed comment to:
"Determines which implementation to use ..."

but... it is not being used for encoding (writing) anymore. always writing with symfony.

so also changed it to "... for parsing YAML."

---

and tried to explain why we always use Symfony for writes (encoding).

Please look at this interdiff carefully.

YesCT’s picture

Assigned: YesCT » Unassigned
Issue summary: View changes

made the issue @alexpott asked for in #249
#2597762: Add a new testbot with (only) Symfony yaml parser (not the pecl one)

That blocks this issue, yes?

------

put the upstream issue in the issue summary.

I think I'm done for now. Unassigning.

YesCT’s picture

I asked about the static methods in IRC (and looked up static http://php.net/manual/en/language.oop5.static.php )

neclimdul responded

the decode method is also static so there is no object instance
I played with making yaml decoding a service with an instance but it was way to disruptive and caused problems because it needs to work for building the container.
we can actually solve that circular reference now i think but at the time we couldn't so everything is static

neclimdul’s picture

neclimdul’s picture

Status: Needs work » Needs review
FileSize
39.68 KB

man, sometimes I can't even roll a patch.

david_garcia’s picture

+1. I've testing for a while now on our dev environments.

Just note that it helped discover broken Yaml's on some contrib because the PECL parser seems to be more strict - or at least does not behave 100% as the symfony parsers.

neclimdul’s picture

This and the tests it provides are now a bit more important. Drush changed its composer file so it now may be installing Symfony 3.0's Yaml component.
https://github.com/drush-ops/drush/pull/2003

Because of how autoloading works, this means when we have invalid YAML going forward, Drush will be broken.
Example: #2674198: @menu_name in menu_links migration_template is invalid yaml

neclimdul’s picture

Just a reroll. NW because I don't know what the path forward is and #2674198 would cause it to fail.

neclimdul’s picture

Status: Needs work » Needs review

In hindsight, the status didn't matter. the testbot issue is still open.

neclimdul’s picture

Version: 8.0.x-dev » 8.2.x-dev

Just updating since this would have to be applied to 8.2.x first. Still applies cleanly and does its thing. It looks like its being tested by testbot too as there are just shy of 2000 more assertions in the last test run when compared to the HEAD run on the same day.

Mixologic’s picture

Issue tags: +drupalci_environments
neclimdul’s picture

neclimdul’s picture

#272 still applies to 8.0 but #2686207: Convert simpletest kernel tests in modules A-I to phpunit broke this for 8.1.x and 8.2.x so here is the reroll.

neclimdul’s picture

Sorry user error. wrong parameters to my reroll script ends up with all the difference between 8.1 and 8.2 :(

xjm’s picture

Issue tags: -rc target
xjm’s picture

Discussed with @neclimdul and marking for framework manager review for some visibility.

joelpittet’s picture

Issue summary: View changes

They seem to be aligning themselves with PECL as well:
http://symfony.com/blog/new-in-symfony-3-1-yaml-deprecations

This to the issue summary

neclimdul’s picture

Its that time again. Time for a conflict reroll.

#2380293: Properly inject services into ModuleInstaller shuffled/added some use imports so that chunk conflicted. no changes.

neclimdul’s picture

Title: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available » [PP-1] Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available
Status: Needs work » Postponed
neclimdul’s picture

Title: [PP-1] Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available » Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available
Status: Postponed » Needs review

Lets try that again.

neclimdul’s picture

Any chance of getting that framework manager review?

jibran’s picture

Status: Needs review » Reviewed & tested by the community

I read today in https://docs.google.com/document/d/1-_AgTnZu9e2iAuEpnp262mguFInNaiIcSxsM...

issues with 'Needs framework manager review' tag can go RTBC before that review is given. It will be done as part of the RTBC/commit review

So setting it to RTBC.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
0 bytes

use statement conflict in moduleinstaller.

neclimdul’s picture

welcome to late night patch rerolls where I forget to diff off origin and make empty patches.

neclimdul’s picture

Noticed the test wasn't catching a yaml file bug in my modules directory because the directory was one ../ off.

neclimdul’s picture

I forgot we have a follow up for contrib. Rather then continuing to fix it I'll revert that change and make a patch there.

Marking RTBC because this is the same patch jibran RTBC'd with the exception of the use statement fixes.

jibran’s picture

+1 for RTBC.

catch’s picture

Assigned: Unassigned » alexpott

I haven't done an end-to-end review of the patch yet, but I like the approach in general. We use the PECL extension to read when we can, we use Symfony to write all the time so that things are always consistent that way.

While we have filecache and similar, those became necessary because YAML parsing is so slow, so improving the raw uncached performance still seems worth it to me.

Assigning to @alexpott since he had more concerns with this than me originally.

catch’s picture

Assigned: alexpott » Unassigned
Issue tags: -Needs framework manager review

Spoke to @alexpott and all his objections are covered by writing with Symfony YAML always. So unassinging and removing framework manager review tag.

alexpott’s picture

I wonder what has changed to cause the fail - the error looks related to the patch and not random.

Mixologic’s picture

Not related to the patch: related to php7 being on the bleeding edge of 7.x php branch: #2764687: IpAddressBlockingTest is failing on DrupalCI

neclimdul’s picture

Status: Needs work » Reviewed & tested by the community

That explains why I couldn't find anything with the couple hours I spent trying to track it down :) oh well, reverting status.

alexpott’s picture

Config.Drupal\config\Tests\ConfigSingleImportExportTest
✓		- setUp
✗	
testImport
fail: [Other] Line 48 of core/modules/config/src/Tests/ConfigSingleImportExportTest.php:
"The import failed with the following message: Malformed inline YAML string ({{{) at line 1 (near &quot;{{{&quot;)" found
✓		- testImportSimpleConfiguration
✓		- testExport

This is the fail I was wondering about.

Mixologic’s picture

Aha. I had to hunt around to find it: https://www.drupal.org/pift-ci-job/364955

Its strange that it doesnt fail now.

alexpott’s picture

Issue tags: +Needs change record

So I think we need a change record that details the added support for the PECL YAML extension and it probably needs to explain the only unmitigated change (#111) - the fact that Drupal does not support dots in anchor names. Anchors are very very rarely used so I think this is acceptable.

alexpott’s picture

Issue tags: +8.2.0 release notes
Crell’s picture

In the change notice, please include the benefits of doing so, ie, whether you'd want to add that extension on production. (You know, for Drupal hosts... :-) )

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Setting to 'needs work' for the CR.

neclimdul’s picture

Mixologic’s picture

Status: Needs work » Needs review
joelpittet’s picture

Issue tags: -Needs change record

Stubbed a draft CR for you to all tweak
https://www.drupal.org/node/2769555

neclimdul’s picture

Status: Needs review » Needs work

The last submitted patch, 308: add_a_drupal_yaml-1920902-308.patch, failed testing.

catch’s picture

That looks like a real failure:

"The import failed with the following message: Malformed inline YAML string ({{{) at line 1 (near &quot;{{

https://www.drupal.org/pift-ci-job/380935

neclimdul’s picture

Actually the fact that passes anywhere is weird. "Malformed inline YAML string" is from Symfony and that test is suppose to be testing that the error from the parser is triggered. For a different parser the proxied error is going to be slightly different. it should just assert "The import failed with the following message: "

neclimdul’s picture

A big update of the patch to catch all the new inclusions of Yaml from Component since we've just been re-rolling for a while now. This should force the failure in in #310 because it was one of those locations. I've adjusted the test to assert the static part of the error which is just "The import failed with the following message: "

Status: Needs review » Needs work

The last submitted patch, 312: add_a_drupal_yaml-1920902-312.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
54.84 KB

bah... core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php needs a binary diff. same patch with --binary. Interdiff would be the same with the addition of an unreadable binary hash for that file where Component is being changed to Core.

neclimdul’s picture

Updated the CR to contain more information.

Mixologic’s picture

i would RTBC, but IM unsure if #314 was supposed to pass or not.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Yeah #314 suppose to pass.

alexpott’s picture

@neclimdul I still don't understand why the failure was random.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Discovery/YamlDiscovery.php
    @@ -0,0 +1,25 @@
    +<?php
    +
    +namespace Drupal\Core\Discovery;
    +
    +use Drupal\Component\Discovery\YamlDiscovery as ComponentYamlDiscovery;
    +use Drupal\Core\Serialization\Yaml;
    +
    +/**
    + * Provides discovery for YAML files within a given set of directories.
    + */
    +class YamlDiscovery extends ComponentYamlDiscovery {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function findAll() {
    +    $all = array();
    +    foreach ($this->findFiles() as $provider => $file) {
    +      $all[$provider] = Yaml::decode(file_get_contents($file));
    +    }
    +
    +    return $all;
    +  }
    +
    +}
    

    Why does this exist? I.e. why are bypassing the file caching in \Drupal\Component\Discovery\YamlDiscovery?

    This addition, and the related changes, do not seem necessary for the patch.

  2. +++ b/core/modules/config/src/Controller/ConfigController.php
    @@ -3,11 +3,11 @@
    -use Drupal\Component\Serialization\Yaml;
    ...
    +use Drupal\Core\Serialization\Yaml;
    

    The change notice should tell people to swap out the component Yaml for the core Yaml to behave as expected.

  3. +++ b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
    @@ -45,7 +45,8 @@ public function testImport() {
    -    $this->assertText('The import failed with the following message: Malformed inline YAML string ({{{) at line 1 (near &quot;{{{&quot;)');
    +    // Assert the static portion of the error since different parsers could give different text in their error.
    +    $this->assertText('The import failed with the following message: ');
    

    This has the potential to cause contrib tests to fail - should be mentioned in the CR.

  4. +++ b/core/modules/image/migration_templates/d6_imagecache_presets.yml
    @@ -18,7 +18,7 @@ process:
    -      - '@plugin'
    +      - "@plugin"
    

    Is this actually a required change for this to work?

  5. +++ b/core/modules/system/src/Tests/Installer/StandardInstallerTest.php
    @@ -55,15 +55,15 @@ public function testStandardConfig() {
    -    $skipped_config['filter.format.restricted_html'][] = ' - anonymous';
    +    $skipped_config['filter.format.restricted_html'][] = '- anonymous';
    

    This has the potential to cause contrib tests to fail should be mentioned in the CR.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Serialization/Yaml.php
@@ -0,0 +1,28 @@
+      $class = Settings::get('yaml_parser_class')) {

This setting has no documentation and is untested.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
9.14 KB
60.39 KB

1) I think it was just written before FileCache existed. Refactored Component to allow decoding to be replaced so the Component FileCache logic can be used. Additionally made sure new uses of YamlDiscovery since this was written use the Core version now.
2) 3) 5) CR Updated. Additionally documented YamlDiscovery change from 1)
4) I think that was just fixed in a different issue and that's the artifact of a merge. " is more consistent with our other YAML files and I believe what will be output by both Serializers but ' isn't wrong. Reverted that block.

Test and documentation for setting in interdiff.

Status: Needs review » Needs work

The last submitted patch, 321: add_a_drupal_yaml-1920902-321.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
60.42 KB

gosh darn --binary...

alexpott’s picture

I can't see if the testing part of #320 has been addressed?

neclimdul’s picture

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the test @neclimdul. Everything looks good now! Nice one.

I've reviewed and tested this patch extensively. The CR looks good - telling developers and site owners how this change impacts them. I've discussed this change with @catch and we agreed that whilst this does mean the developers should update their code to use the core classes, if they continue to use the component classes nothing with break, just the new setting won't work. This is an acceptable trade-off for the potential cold-cache performance benefits.

I've not being involved in any of the more recent code so I don't consider this my patch.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Really, really nice to be able to commit this one.

Committed a0a530f and pushed to 8.2.x. Thanks!

  • catch committed a0a530f on 8.2.x
    Issue #1920902 by neclimdul, larowlan, alexpott, chx, sun, jthorson,...
anavarre’s picture

Thanks for sticking to this issue for so long @neclimdul!

Wim Leers’s picture

Yes, incredible persistence, @neclimdul!

neclimdul’s picture

Thanks guys! Your kind words really recharge the old batteries after a long haul. :)

It takes a village though and you guys are a great one. A round of applause for the mountain of effort going on from everyone else because there where a ton of reviews, fixes and 2 years of effort before I even rolled a patch! :)

  • catch committed a0a530f on 8.3.x
    Issue #1920902 by neclimdul, larowlan, alexpott, chx, sun, jthorson,...

  • catch committed a0a530f on 8.3.x
    Issue #1920902 by neclimdul, larowlan, alexpott, chx, sun, jthorson,...

Status: Fixed » Closed (fixed)

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