Skip to content
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

Php ampersand fix #4705

Merged
merged 3 commits into from
May 20, 2022
Merged

Conversation

jeroenpost86
Copy link
Contributor

Issue #, if available:

Description of changes:
Fixed the regex for T_AMPERSAND_FOLLOWED_BY_VAR_OR_VARARG and T_AMPERSAND_NOT_FOLLOWED_BY_VAR_OR_VARARG as this caused issues with the syntax checker

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jjpost added 3 commits May 10, 2022 16:19
* 'master' of https://github.com/ajaxorg/ace:
  fix: Made commas be tokenized as punctuation operator instead of text in JSON (ajaxorg#4703)
  fix: Multiple Partiql and Amazon Ion textual notation fixes (ajaxorg#4686)
  fix: Updated PHP mode to support PHP8.1 syntax (ajaxorg#4696)
  release v1.5.0
  chore: use npm changelog in release script (ajaxorg#4698)
  feat: Added ability to configure certain format options for beautify extension
  fix: Render bidirectional unicode characters as control characters (ajaxorg#4693)
@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #4705 (d59d450) into master (4c4883a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4705   +/-   ##
=======================================
  Coverage   71.19%   71.19%           
=======================================
  Files         553      553           
  Lines       55598    55598           
  Branches    10392    10392           
=======================================
  Hits        39584    39584           
  Misses      16014    16014           
Flag Coverage Δ
unittests 71.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c4883a...d59d450. Read the comment docs.

@andrewnester
Copy link
Contributor

@jeroenpost86 could you provide an example of code that caused errors before?

@andrewnester andrewnester self-requested a review May 19, 2022 19:43
@jeroenpost86
Copy link
Contributor Author

jeroenpost86 commented May 19, 2022

This some example code with the issues:

<?php

    function array_to_stdClass(&$array){
        $tmp = $array;
        $array = new stdClass();
        # false positive, works just fine
        foreach($tmp as $k => $v) $array->$k = (!is_object($v) && !is_array($v)) ? $v:array_to_stdClass($v);
        return $array;
    }
    function stdClass_to_array(&$stdClass){
        $tmp = $stdClass;
        $stdClass = array();
        # false positive, works just fine
        foreach($tmp as $k => $v) $stdClass[$k] = (!is_object($v) && !is_array($v)) ? $v:stdClass_to_array($v);
        return $stdClass;
    }

It is because T_AMPERSAND_FOLLOWED_BY_VAR_OR_VARARG wasn't defined properly.

Here is the same code running in an editor after the fix: https://onlinephp.io/c/2d00a

@andrewnester
Copy link
Contributor

I'm not sure I fully understand the issue. If I use this sample in kitchen-sink here https://ace.c9.io/build/kitchen-sink.html
everything parsed correctly and no errors reported. Then what this PR is trying to solve?

@jeroenpost86
Copy link
Contributor Author

jeroenpost86 commented May 20, 2022

I'm not sure I fully understand the issue. If I use this sample in kitchen-sink here https://ace.c9.io/build/kitchen-sink.html everything parsed correctly and no errors reported. Then what this PR is trying to solve?

The kitchen sink is running an older version of the PHP parser. When I open the kitchen sink straight from the repo with node ./static.js the error shows up, but not on the link you posted.
Also, the kitchen sink doesn't support PHP8 yet, while my last PR with PHP8 support was merged last week (what caused this specific issue).

PHP 8 stuff:

<?php

// Ace Editor PHP 8.1 demo

enum UserStatus: string
{
    case Pending = 'P';
    case Active = 'A';
    case Suspended = 'S';
    case CanceledByUser = 'C';

    public function label(): string
    {
        return match($this) {
            static::Pending => 'Pending',
            static::Active => 'Active',
            static::Suspended => 'Suspended',
            static::CanceledByUser => 'Canceled by user',
        };
    }
}

foreach (UserStatus::cases() as $case) {
    printf('<option value="%s">%s</option>'."\n", $case->value, $case->label());
}


class Service
{
    private Logger $logger;
    public readonly Status $status;
    final public const XX = "foo";

    public function __construct(
        Logger $logger = new NullLogger(), Status $status = new Status(),
    ) {
        $this->logger = $logger;
        $this->status = $status;
        $foo = $this->foo(...);

        $var ??= 0; // Syntax error, unexpected '='
        return $var + 1;
    }
    
    #[\Assert\All(
        new \Assert\NotNull,
        new \Assert\Length(min: 5))
    ]
    public string $name = '';

}

Screenshot of locally running kitchen sink:

git clone https://github.com/ajaxorg/ace.git ace
cd ace
node ./static.js

image

@andrewnester
Copy link
Contributor

Okay, got it, so it's essentially a followup for 8.1 update which was merged but not yet release. Thanks!

@andrewnester andrewnester merged commit d59c22b into ajaxorg:master May 20, 2022
@jeroenpost86
Copy link
Contributor Author

Okay, got it, so it's essentially a followup for 8.1 update which was merged but not yet release. Thanks!

Correct. Sorry for the confusion. I wrongly assumed the 8.1 update was already released :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants