Add yaml parsing & emitting support #707

NikOsint · 2022-12-18T18:49:56Z

KPHP YAML

Solution for Issue #639

Implementation of 4 functions from PHP YAML using yaml-cpp:

yaml_emit_file(string $filename, mixed $data): bool
yaml_emit(mixed $data): string
yaml_parse_file(string $filename, int $pos = 0): mixed
yaml_parse(string $data, int $pos = 0): mixed

C++ tests are written and implemented.

~~There is one major issue: strings like "10", "98.13' are emitted without quotes, so then they are parsed as integers and doubles respectively.~~

More informative README can be found here.

Tsygankov-Slava

I have found several places where something can be improved. It would be great if you took these comments into attention.

runtime/yaml.cpp

NikOsint · 2022-12-23T18:31:26Z

@Tsygankov-Slava Thank you for your review! All the comments were taken into account and the corresponding parts of the code were corrected. Some other small improvements were done.

For now, the problem of emitting number strings such as "10", "5.34" without quotes remains due to poor documentation of yaml-cpp library. I will let you know when a solution is found.

… execute

…_run_once.py:43

NikOsint · 2023-02-27T19:15:50Z

@Tsygankov-Slava @unserialize The problem of emitting numbers and numbers-as-strings has been solved.

Converting from mixed to a YAML document has been rewritten without use of yaml-cpp as it does not provide any way to emit strings with quotes and numbers without.

Also now booleans are parsed&emitted correctly (as booleans, not strings or integers).

More C++ tests has been written and a complex PHP test has been implemented, too.

For the PHP test to work, a 'yaml.so' extension has been added to the tests/python/lib/kphp_run_once.py file. Without it, the test would throw an error when running with PHP because PHP does not have its YAML module by default.

In the PHP test, serialize() method is used to output mixed. var_dump() is not used as its output is different in PHP and KPHP.

NikOsint · 2023-02-28T17:01:15Z

Merged VKCOM:master into my master branch so that there are no conflicts.

Tsygankov-Slava

I looked your PR and found a few points that you might want to fix.

runtime/yaml.cpp

runtime/yaml.h

runtime/yaml.cpp

tolk-vm · 2023-03-09T07:09:53Z

I'll try to review it next week

tests/phpt/yaml/01_complex.php

tests/python/lib/kphp_run_once.py

tests/phpt/yaml/03_yaml_escaping.php

tolk-vm · 2023-10-19T15:13:05Z

builtin-functions/_functions.txt

@@ -1652,3 +1652,8 @@ class DateTimeImmutable implements DateTimeInterface {
 }

 function getenv(string $varname = '', bool $local_only = false): mixed;
+
+function yaml_emit_file ($filename ::: string, $data ::: mixed) ::: bool;


(string $filename, mixed $data) and others, no :::, just type hints. Same for return — just ':' after ')'

tolk-vm · 2023-10-19T15:14:06Z

runtime/runtime.cmake

@@ -49,6 +49,11 @@ prepend(KPHP_RUNTIME_PDO_PGSQL_SOURCES pdo/pgsql/
        pgsql_pdo_emulated_statement.cpp)
 endif()

+if (YAML)
+prepend(KPHP_RUNTIME_YAML_SOURCES /


missing identation in if

I basically copied this if from above ifs, there is also no identation
should I fix it there too?

it honestly looks worse with indentation inside the if-clause
I think that's why there is no indentation in above ifs

tolk-vm · 2023-10-19T15:14:25Z

runtime/streams.h

@@ -6,7 +6,7 @@

 #include "runtime/kphp_core.h"

-using Stream =mixed;
+using Stream = mixed;


Very strange change in this PR

just noticed a missing space there

deleted this change

tolk-vm · 2023-10-19T15:14:45Z

runtime/yaml.cpp

+
+#include "runtime/optional.h"
+#include "runtime/streams.h"
+#include "yaml.h"


"runtime/yaml.h"

tolk-vm · 2023-10-19T15:15:42Z

runtime/yaml.cpp

+  if (node.IsScalar()) {
+    const string string_data(node.as<std::string>().c_str());
+    // check whether the primitive is put in quotes in the source YAML
+    const bool string_data_has_quotes = (source[node.Mark().pos] == '"' && source[node.Mark().pos + string_data.size() + 1] == '"');


What about single quotes? I suppose, they also mean a string in yaml

tolk-vm · 2023-10-19T15:17:51Z

runtime/yaml.cpp

+/*
+ * print tabs in quantity of nesting_level (used to print nested YAML entries)
+ */
+static string yaml_print_tabs(const uint8_t nesting_level) noexcept {


Why do you use uint8_t for a number? There is no reason not to use just int unless memory alignment is a case you care for

well, I thought it is unrealistic to exceed 255 levels of nesting in a YAML document, so I decided to limit memory for variable a little bit
and uint8_t is safer in terms of sign

changed to int

tolk-vm · 2023-10-19T15:18:44Z

runtime/yaml.cpp

+  string escaped_data;
+  for (size_t i = 0; i < data.size(); ++i) {
+    if (data[i] == 10) { // line feed - code 10
+      escaped_data.push_back(92); // backslash


'\' instead of 92 (and below)?

tolk-vm · 2023-10-19T15:20:51Z

runtime/yaml.cpp

+static string yaml_escape(const string &data) noexcept {
+  string escaped_data;
+  for (size_t i = 0; i < data.size(); ++i) {
+    if (data[i] == 10) { // line feed - code 10


extract data[i] to a separate (char) variable: you use in dozen of times

why do you compare char with a numeric code, why not `data[i] == '\n' and get rid of comments everywher?

tolk-vm · 2023-10-19T15:22:37Z

runtime/yaml.cpp

+    } else if (data.is_bool()) {
+      buffer = (data.as_bool()) ? string("true") : string("false");
+    }
+    string_data.append(buffer);


What for do you need buffer, why not to append into string_data directly?

tolk-vm · 2023-10-19T15:25:09Z

runtime/yaml.cpp

+mixed f$yaml_parse_file(const string &filename, int pos) {
+  if (filename.empty()) {
+    php_warning("Filename cannot be empty");
+    return {};


In PHP, false in returned on failure

tolk-vm · 2023-10-19T15:30:20Z

There is one major issue: strings like "10", "98.13' are emitted without quotes, so then they are parsed as integers and doubles respectively.

Is this issue (from PR description) still actual? I can't see such a behaviour looking at source code

NikOsint · 2023-10-22T12:21:42Z

There is one major issue: strings like "10", "98.13' are emitted without quotes, so then they are parsed as integers and doubles respectively.

Is this issue (from PR description) still actual? I can't see such a behaviour looking at source code

No, it is outdated, now everything in double quotes is emitted as string
Thanks for this remark, updated description

…of char escaping

kphp-yaml v0.1: parse, emit, parse_file, emit_file, c++ tests

e06286d

Danil42Russia linked an issue Dec 19, 2022 that may be closed by this pull request

Support for parsing yaml files. #639

Open

tolk-vm requested review from quasilyte and tolk-vm December 19, 2022 10:02

Tsygankov-Slava reviewed Dec 19, 2022

View reviewed changes

tolk-vm requested a review from DrDet December 20, 2022 19:11

khhp-yaml: codestyle fixes & memory optimizations

6cd1ccf

tolk-vm added the missing functionality Missing PHP functions, classes, constants and etc label Feb 2, 2023

Nikolay Osintsev added 6 commits February 26, 2023 11:27

yaml: add empty string tests and number-as-string tests

da3ee16

yaml: fix number-as-string parsing and rewrite mixed_to_yaml() function

4e9cc2e

kphp-yaml: fix boolean emitting & parsing

a4422d8

kphp-yaml: add PHP test

d7322b5

kphp-yaml: add yaml.so extension to kphp_run_once.py for PHP tests to…

48febd9

… execute

kphp yaml: return an accidentally deleted line (tests/python/lib/kphp…

51771d2

…_run_once.py:43

Nikolay Osintsev and others added 3 commits February 28, 2023 19:52

kphp-yaml: temporarily deleted changes in _functions.txt

66131a7

Merge branch 'VKCOM:master' into master

dc33da4

kphp-yaml: bring back changes in _functions.txt [conflict resolved]

3c1d0dd

NikOsint requested review from Tsygankov-Slava and removed request for quasilyte, DrDet and tolk-vm March 6, 2023 10:02

Tsygankov-Slava reviewed Mar 6, 2023

View reviewed changes

Danil42Russia reviewed Mar 10, 2023

View reviewed changes

tests/phpt/yaml/01_complex.php Outdated Show resolved Hide resolved

Danil42Russia reviewed Mar 10, 2023

View reviewed changes

tests/python/lib/kphp_run_once.py Outdated Show resolved Hide resolved

Nikolay Osintsev added 2 commits March 12, 2023 00:27

yaml: delete obvious comments in runtime/yaml.h

c456682

yaml: support for empty arrays, new comments, codestyle fixes

a5adf3f

NikOsint and others added 4 commits May 29, 2023 01:23

Merge branch 'VKCOM:master' into master

d1f58a4

YAML: add PHP tests for symbols that should be escaped

3dcf68a

Merge branch 'VKCOM:master' into master

c0f6174

Merge branch 'VKCOM:master' into master

562f7f6

Danil42Russia reviewed Sep 17, 2023

View reviewed changes

tests/phpt/yaml/03_yaml_escaping.php Outdated Show resolved Hide resolved

Nikolay Osintsev and others added 8 commits September 19, 2023 15:45

yaml: add a linebreak in test 03

343a9c9

Merge branch 'master' of github.com:NikOsint/kphp

4d71001

yaml: make yaml an extension, turning on by CMake option `-DYAML=ON

ecf7065

Merge branch 'VKCOM:master' into master

1f511fb

yaml: minor changes

361adda

Merge branch 'VKCOM:master' into master

1ed14d1

yaml: move cpp tests to common dir, delete extensions-tests dir

29797e4

Merge branch 'VKCOM:master' into master

0efca0a

andarut requested a review from Danil42Russia October 12, 2023 10:19

Danil42Russia assigned andarut Oct 18, 2023

tolk-vm reviewed Oct 19, 2023

View reviewed changes

Merge branch 'VKCOM:master' into master

e25efd2

NikOsint closed this Oct 22, 2023

NikOsint reopened this Oct 22, 2023

Nikolay Osintsev and others added 9 commits October 23, 2023 02:18

yaml: fix issues after @unserialize review, mostly in implementation …

12b31da

…of char escaping

yaml: little codestyle fixes

97474f6

Merge branch 'VKCOM:master' into master

f320626

yaml: add test for strings in single quotes

c07c1aa

Merge branch 'VKCOM:master' into master

a842143

Merge branch 'VKCOM:master' into master

5b3d285

Merge branch 'VKCOM:master' into master

3bf24b8

Merge branch 'VKCOM:master' into master

a8117ec

Merge branch 'VKCOM:master' into master

1606199

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add yaml parsing & emitting support #707

Add yaml parsing & emitting support #707

NikOsint commented Dec 18, 2022 •

edited

Loading

Tsygankov-Slava left a comment

NikOsint commented Dec 23, 2022

NikOsint commented Feb 27, 2023 •

edited

Loading

NikOsint commented Feb 28, 2023

Tsygankov-Slava left a comment

tolk-vm commented Mar 9, 2023

tolk-vm Oct 19, 2023

tolk-vm Oct 19, 2023

NikOsint Oct 22, 2023

NikOsint Nov 14, 2023

tolk-vm Oct 19, 2023

NikOsint Oct 22, 2023

NikOsint Oct 29, 2023

tolk-vm Oct 19, 2023

tolk-vm Oct 19, 2023

tolk-vm Oct 19, 2023

NikOsint Oct 22, 2023 •

edited

Loading

NikOsint Oct 29, 2023

tolk-vm Oct 19, 2023

tolk-vm Oct 19, 2023

tolk-vm Oct 19, 2023

tolk-vm Oct 19, 2023

tolk-vm commented Oct 19, 2023

NikOsint commented Oct 22, 2023

Add yaml parsing & emitting support #707

Are you sure you want to change the base?

Add yaml parsing & emitting support #707

Conversation

NikOsint commented Dec 18, 2022 • edited Loading

KPHP YAML

Tsygankov-Slava left a comment

Choose a reason for hiding this comment

NikOsint commented Dec 23, 2022

NikOsint commented Feb 27, 2023 • edited Loading

NikOsint commented Feb 28, 2023

Tsygankov-Slava left a comment

Choose a reason for hiding this comment

tolk-vm commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikOsint Oct 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolk-vm commented Oct 19, 2023

NikOsint commented Oct 22, 2023

NikOsint commented Dec 18, 2022 •

edited

Loading

NikOsint commented Feb 27, 2023 •

edited

Loading

NikOsint Oct 22, 2023 •

edited

Loading