fix: add string escape for encapsed

[mgrip]

#29

Unfortunately I don't think we can fix #253 right now. Maybe we can utilize the raw attribute from the parser? But node.value is exactly the same whether its a literal line break or escaped

[mgrip]

I'm going to delete that failing test for now - there's already an open issue for it #253

[czosel]

Hmm, are you sure?

PHP Script :

<?php
'You can also have embedded\n newlines in 
strings this way as it is
okay to do';

AST Structure :

{
  "kind": "program",
  "children": [
    {
      "kind": "string",
      "value": "You can also have embedded\\n newlines in \nstrings this way as it is\nokay to do",
      "raw": "'You can also have embedded\\n newlines in \nstrings this way as it is\nokay to do'",
      "isDoubleQuote": false
    }
  ],
  "errors": []
}

glayzzle/php-parser#123

[mgrip]

Ya I saw the same thing when using the parser playground, but when I actually debugged in the code node.value was exactly the same

[czosel]

Oh, ok - maybe this changed in the meantime. Using raw definitely sounds like a plan.

[czosel]

How is this working? I've just looked at the implementation of makeString, but I don't see how passing an empty quote is affecting the escaping ...

[mgrip]

I don't think it would affect the escaping - but when the string is part of an encapsed node, that parent node is what's responsible for printing the actual quote. so if you had $test = "test ${string}"; - that actually gets broken up into multiple strings. the parent node is an encapsed node (which is responsible for printing the quote), and the children are test and ${string} i think - which we wouldn't want to re-print quotes for

[czosel]

Right - and is the change affecting the tests in any way?

[mgrip]

I don't think so? Other than keeping escaped line breaks escaped (which we're not currently doing)

[alexander-akait]

Why raw sounds like a pain?

[mgrip]

@czosel @evilebottnawi switched to use node.raw - let me know what you think. Would now fix #253 as well

[czosel]

I'm usually not too much into using Regexes, but here it might be shorter:

if (['"', "'"].includes(stringValue[0])) {
  stringValue = str.replace(/^["'](.*)["']$/, '$1');
}

[mgrip]

🙏 thank you - im terrible with regex haha

[czosel]

Me too, i basically copied this from StackOverflow 😂

[mgrip]

hrm that doesn't seem to be working for me though

[czosel]

Hmm, looks ok to me:

strip = str => str.replace(/^["'](.*)["']$/, '$1')
strip("\"foo\"")
"foo"
strip("'foo'")
"foo"
strip("'foo' a")
"'foo' a"

[mgrip]

for whatever reason this test is failing

echo "This spans
multiple lines. The newlines will be
output as well";

[czosel]

Oh, it was lacking multiline support. Try ^["']([\s\S]*)["']$

[czosel]

I think we're slowly reaching the point where the Regex becomes so cryptic that it's better to keep the longer, non-regex version 😉

[czosel]

Maybe @evilebottnawi can answer that 😄

[czosel]

Indentation is strange here, right? (Has not much to do with this PR though)

[mgrip]

agreed. i think thats just the way our assign node breaks though if we go over the line limit. maybe we need to think about an exception for strings or something? not sure best way to handle it

[czosel]

Yep, that might be an idea. Let's open a new issue about this, it's probably not very urgent.

[czosel]

👍 Looks good, with or without regex 😉