-
Notifications
You must be signed in to change notification settings - Fork 791
Rewrite and flesh out string interpolation docs #2000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor things, looks good to me overall.
language/types/string.xml
Outdated
<link linkend="language.types.string.parsing.simple">simple</link> one and a | ||
<link linkend="language.types.string.parsing.complex">complex</link> one. | ||
The simple syntax is the most common and convenient. It provides a way to | ||
<link linkend="language.types.string.parsing.simple">basic</link> one and an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pages are still named simple and complex, should those be renamed? I'm also not sure this needs to be renamed at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to change them in case other pages link to here. But I could check and change them
language/types/string.xml
Outdated
]]> | ||
</screen> | ||
</informalexample> | ||
The <link linkend="language.types.string.parsing.complex">advanced</link> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The basic one would be fine too for the first example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I was trying to have an example to demonstrate the reason why it is deprecated, so I copied the RFC example, but if you have a better example I'll take it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message only mentions {$var}
because that will work for all versions of ${var}
, like ${var['foo']}
. That's not valid in the basic syntax (when translating 1:1), where the '
need to be omitted: $var[foo]
. When only simple variables are used, $var
should be fine as well.
|
||
<simpara> | ||
If a dollar sign (<literal>$</literal>) is encountered, the parser will | ||
greedily take as many tokens as possible to form a valid variable name. | ||
Enclose the variable name in curly braces to explicitly specify the end of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems kind of useful. Maybe move it to the advanced section instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would code using the advanced syntax use this? And isn't that part of the deprecation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this comment is saying that by enclosing the whole expression with {$...}
, the parser doesn't need to guess when the expression is done (or rather stop after the first dimension) but will look for the ending }
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this comment is saying that by enclosing the whole expression with
{$...}
, the parser doesn't need to guess when the expression is done (or rather stop after the first dimension) but will look for the ending}
instead.
No, this comment is referring to the ${...}
syntax. If you read the current docs that's how it's used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section seems clear to me as @Girgias has changed it, enclosing just the variable name in brackets (${....}
) was deprecated, but still explained in the warning that comes a bit later.
language/types/string.xml
Outdated
curly braces surrounding the expression. | ||
</simpara> | ||
|
||
<sect4 xml:id="language.types.string.parsing.simple"> | ||
<title>Simple syntax</title> | ||
<title>Basic syntax</title> | ||
|
||
<simpara> | ||
If a dollar sign (<literal>$</literal>) is encountered, the parser will | ||
greedily take as many tokens as possible to form a valid variable name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find that statement a little misleading. The syntax isn't greedy, e.g. only one array dimension is allowed. https://3v4l.org/pJF8E
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well brilliant, this is already the existing docs but might as well fix them now. Do you have better wording? I can try to come up with one otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like:
The basic syntax only allows for single-dimensional array or property accesses.
$foods = ['apples', 'bananas', 'lettuce', 'spinach']; echo "$foods[0]\n"; // apples $foods = ['fruits' => ['apples', 'bananas'], 'vegetables' => ['lettuce', 'spinach']]; echo "$foods[fruits][0]\n"; // Warning: Array to string conversion in /in/hcu76 on line 7 // Array[0]To use multi-dimensional array or property accesses, use the advanced syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find that statement a little misleading. The syntax isn't greedy, e.g. only one array dimension is allowed. https://3v4l.org/pJF8E
Actually this statement is only about the name of a variable, not any following dimension. So I think it's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what that statement is really about (there are still places in the docs where it talks about "variables" but means "values", what I consider grossly wrong). However, if it is only about the identifier, the word "token" doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, braces don't change how the variable name itself is tokenized, so it's not even correct in that context.
33370e5
to
a08df37
Compare
I spent some more time again, can you please have another look @iluuu1994 @cmb69 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
I'm not convinced that this is a major improvement (but certainly causes quite some work for translators). Wouldn't it be better to have a precise description (like https://github.com/php/php-langspec/blob/master/spec/09-lexical-structure.md#string-literals)?
language/types/string.xml
Outdated
<warning> | ||
<para> | ||
The basic syntax does not permit access to an array entry which has a key | ||
containing a minus (<literal>-</literal>) sign. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes much sense to point out that minus is not allowed, since AFAIK only valid identifier characters are allowed anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are proably others yes, but when I was creating my example I used the array key string-key
which when I tried to interpolate didn't work. So I changed the -
to an _
, however refering to that key is possible via the normal semantics.
language/types/string.xml
Outdated
// Won't work, outputs: C:\folder\{fantastic}.txt | ||
echo "C:\folder\{$great}.txt" | ||
// Works, outputs: C:\folder\fantastic.txt | ||
echo "C:\\folder\\{$great}.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a fix for https://bugs.php.net/81635. Not sure if we should just drop that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay I'll readd it then
Having a more precise description would be best, but even that page has a dead link to what it refers to as "simple expression", and we would need to define what a name is somewhere. Which might be a good idea to have a section on basic language components, because the amount of pages where we redeclare what a label is in PHP is too many. But this is indeed probably the best course, I'll have another go at it. |
a08df37
to
5b7bcad
Compare
So I've given a go at trying to write a formal definition and removing the clunky descriptions and non-examples. |
language/types/string.xml
Outdated
@@ -683,92 +683,167 @@ EOT; | |||
</sect3> | |||
|
|||
<sect3 xml:id="language.types.string.parsing"> | |||
<title>Variable parsing</title> | |||
<title>Variable substitution</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using "substition" instead of "parsing" is an improvement (but there are 3 occurrences of "parsing" left on that page), but I think "variable substitution" is still not correct; at least with the advanced syntax, it is possible to interpolate the results of evaluating expressions. Speaking of "expression substitution/interpolation" would be too general, though. Not sure what to do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using string interpolation now
</simpara> | ||
|
||
<simpara> | ||
Any scalar variable, array element or object property with a | ||
Any scalar variable, array element or object property | ||
(<modifier>static</modifier> or not) with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
language/types/string.xml
Outdated
This isn't called complex because the syntax is complex, but because it | ||
allows for the use of complex expressions. | ||
The advanced syntax permits the interpolation of | ||
<emphasis>variables</emphasis> with arbitrary access expressions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about "access expressions". Do we define that term somewhere? If not, it might be sensible to do that. Or maybe use "accessors"?
3676194
to
57ef188
Compare
Note to self: related to #2078 |
The conflict is just in an example that should be easy to resolve. Otherwise, this is the last PHP 8.2 documentation issue still open on the tracker. |
This covers the PHP 8.2.0 deprecation of interpolation It also renames simple to basic and complex to advanced as those don't make a judgement on complexity
Also fix token descriptions
57ef188
to
f6d7af4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This covers the PHP 8.2.0 deprecation of interpolation It also renames simple to basic and complex to advanced as those don't make a judgement on complexity.