[gonfunko]

The basics

• I validated my changes

The details

Resolves

• Fixes PHP generator test failure: Lists #1201
• Fixes part of Parentheses incorrectly added/omitted by getAdjusted #5326
• Fixes part of Some orders in generators incorrect #4353

Proposed Changes

This PR improves the reliability of the PHP generator. Previously, the code output for Blockly's built-in generator tests had a number of syntax errors and was not actually valid PHP. Additionally, several of the list-related test cases also failed because PHP passes the built-in array() type by value. As a result, a block construction like this:

...would result in "1, 2, 3" (well, actually result in nothing and a warning about an array to string conversion) because the list is returned from the function by value, leaving the variable unmutated. There were some attempts in the generator to work around this using & to get references to variables, but this was inconsistent, didn't work in cases where the operand was an expression rather than a plain variable, and is invalid on non-variable values.

This PR introduces a ListObject class that is used in place of array(). In PHP, assigning an object to a variable assigns an object handle (~a pointer) to the variable. That's still passed by value, but because the value just identifies an object vs actually being the object, this more or less gets pass by reference semantics, so that newly-generated code for the blocks above gives the expected "test, 2, 3".

This PR also updates the math blocks to use fmod() instead of %, as the latter is only valid for integers and Blockly's math blocks allow decimals as well.

Additionally, some of the Order enum values are corrected for new and existing code, and the Order enum itself is updated to treat function calls and member access as higher precedence than new and clone; in PHP,

$foo = clone $bar[2];

clones the item at index 2 of $bar; it does not clone $bar and assign the item at index 2 of the clone to $foo, because the precedence of [] is higher than clone.

Likewise,

print(new ArrayObject(array(1, 2, 3))[1]);

is handled in PHP 8, but is a syntax error in PHP 7, which we document the generator as supporting. Marking [] as higher precedence than new ensures that the generator emits

print((new ArrayObject(array(1, 2, 3)))[1]);

which is valid in both v7 and v8. Per #5326, the inequality in getAdjusted() has also been corrected so that redundant parentheses will not be emitted in some cases.

Finally, this PR updates the generator to emit <?php at the start of its output by default, which is not optional in PHP. This behavior can however be disabled in case systems that use it were already handling this.

With these changes, the generated code for the PHP generator test is syntactically valid and all assertions pass under both PHP 7.4 and 8.5.

I wrote this entire change, but had LLMs review both the change and the PHP generator broadly for correctness multiple times, which did flag a number of issues which I manually corrected.

Breaking Changes

• By default, PHP code will be emitted with a <?php prefix. Call setOutputOpeningTag(false) on the generator object to revert to the old behavior.
• The generated PHP code, particularly for list blocks, is significantly different following this change.
• The Order enum for PHP has changed to reflect the correct precedence.

In general, if you're just using the built-in generator to generate PHP, no action should be needed beyond perhaps toggling the opening tag behavior; the generated code will just be more correct and should no longer contain syntax errors.

Reason for Changes

The generators should generate valid and correct code.

Test Coverage

The entire generator test suite now passes for PHP.

[jamesfudge]

I've confirmed that the generated PHP code works as expected and it all looks good. The only comment I have is about a potential issue if the generated PHP code was run alongside some other code. However, I'm not sure if this is the kind of thing we want to guard against or do we work on the assumption that the generated code will be run in isolation?

[jamesfudge]

ListObject is defined in the global scope, so there could be a risk of collisions if the generated code is used alongside other code.

Would it be worth giving it a more specific name, such as BlocklyList?

Additionally, maybe we should only declare the class if it doesn't exist, in case the code is run alongside some other generated PHP code, e.g.

if (!class_exists('BlocklyList', false)) {
  class BlocklyList extends ArrayObject {
    // …
  }
}

[gonfunko]

In general we more or less do have to assume that the generated code is run in isolation, because user-defined variable or procedure names, while kept unique within the generated code, have the same issue with conflicts if they were dropped into the middle of an external codebase. I did consider using something like BlocklyList, or a \Blockly namespace, but that felt like it leaked too much in the way of internal implementation details; if you're a student using a service built on Blockly, you very likely have never heard of Blockly, but may well actually be shown the generated code, and there's a potential for confusion there.

[rachel-fenichel]

Structurally LGTM, and the explanations make sense to me.