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

Lack of comment in the doubled class causes "ParseError: syntax error, unexpected 'if' (T_IF), expecting function (T_FUNCTION) or const (T_CONST)" #185

Closed
eugenlisov opened this issue Aug 10, 2020 · 27 comments

Comments

@eugenlisov
Copy link

Hi,

Love AspectMock! I finally managed to start testing stuff which I've previously found untestable.

However, I'm getting a very weird and annoying issue with it, though. I'm getting "ParseError: syntax error, unexpected 'if' (T_IF), expecting function (T_FUNCTION) or const (T_CONST)" without any apparent reason.

image

I've done a bunch of tests to figure out what it could be. I've even went so far as to try to double a completely blank class, just it's definition.

Actually, I've now reproduced it:

Here's the test:
image

Here's the Class
image

And here's the Result::
image

There's absolutely no 'if' in that class. It's as clean as it can be.

At first I thought it was the bootstrap.php file, but it's all correct there:
image

The $kernel is initialized correctly.
image

After a lot work trying to figure it out, I did find a quite easy fix, but which makes no sense.

Just by adding a simple comment "//"after opening the class, makes the test pass. Like this:
image
This is the result:
image

It makes absolutely no sense to me why that fixes it.
I'm ok with that fix for now, as it allows me to focus on the rest of the project. It's not pretty having to add useless comments throughtout the code, but it does the job for now.

It would be nice to know what actually causes this issue and if there's anything more robust I can do, or can be added to AspectMock to avoid this in the future.

Thanks!

@dudapiotr
Copy link

Downgrade to older version of goaop/framework helped me : "goaop/framework": "2.3.2".
There must be some bug.

@eugenlisov
Copy link
Author

Hi @dudapiotr ,

Thanks for the suggestion. I did the downgrade (from 2.3.4) in two different projects and it worked 50%.

  1. A small Laravel project. I did the downgrade and removed the comments. Tests run correctly:
    image

  2. A WP plugin. I did the downgrade, removed the comments. But this time, I get the same errors:
    image
    The class I'm doubling looks like this:
    image
    Once I put back the comment, the tests start running fine:
    image
    The comment looks like this:
    image

So, I'm guessing it's a combination between goaop/framework version and some other package that's not working right. But I'm really not capable of going though the composer.lock and trying to figure it out. There must be a better way than going line by line. I'll stick with the comments in the second project for now

@eugenlisov
Copy link
Author

Found another thing that causes the same issue.
If I import more than one class at the top of the file it fails. If I leave just one, it works.

So, for this I do another workaround: I inline the full namespace of each class.
image
Not pretty, but it does the job.

@eugenlisov
Copy link
Author

It turns out that whatever the bug is, it is causing way more problems than I initially thought.
The most annoying was that it caused other tests that didn't use AspectMock to fail. For crazy reasons ( Unexpected "::"s where there were none, others that I can't remember). So I got to a point where half of the tests (the ones using AM) were working, the other failing. Then, I would disable AspectMock from bootstrap and the other half would start working and the AM ones fail. There was no way to do have a full suite test run from start to finish.

So I finally decided to remove AspectMock from the project and do my test doubles by extending the base classes and overwriting the methods that were calling in other dependencies. That was pretty easy, in my current project at least.
I hope I don't get into the same issues with the other projects where I've started using it. I'm a bit nervous at the thought that I may go in too deep with AspectMock and then end up with tens of tests failing for no reason.. I'll keep an eye on that and if I get a hint of troubles, I'll have to switch to the test double trick I did above. How that won't be the case.

Thanks!

@thornview
Copy link

I'm seeing the same error and have been able to track it down a bit further, but don't know how to fix it.

AspectMock overrides methods by adding an if statement to the beginning of the method. For some reason, it is now adding an if statement to switch statements as well, which is not valid in PHP. Here's a sample I captured using xdebug. AspectMock adds the line if (($__am_res = __amock_before($this, __CLASS__, __FUNCTION__, array(), false)) !== __AM_CONTINUE__) { return $__am_res; } to the code. In this sample you'll see AM has added it to the beginning of the method AND to the switch statement.

 public function hasSignatureFile(): bool
    { if (($__am_res = __amock_before($this, __CLASS__, __FUNCTION__, array(), false)) !== __AM_CONTINUE__) { return $__am_res; }

      switch ($this->getRedcapPid()) { if (($__am_res = __amock_before($this, __CLASS__, __FUNCTION__, array(), false)) !== __AM_CONTINUE__) { return $__am_res; }

    case self::REDCAP_LC_HRPP:
            case self::REDCAP_LC_HRPP_CC:
            case self::REDCAP_LC_PI:
            case self::REDCAP_LC_PI_CC:
                return true;
            default:
                return false;
        }
    }

@sollidsnake
Copy link

This issue seems to be related to the php-parser package. I downgraded it to the version 4.6.0 and now AspectMock works with no issues.

@traceypooh
Copy link

THANK YOU @sollidsnake - this has been plaguing us for months now...
I found we needed to peg 1 hop further back, eg:

"nikic/php-parser": "4.5.0",

which, in turn, holds us back to phpunit v8.

But after days of trying to sort this out (for a newer, focal-based version of our monolith) finally getting non-corruptions for once!

@xfuturomax
Copy link

I found linked issue: goaop/parser-reflection#104

@xfuturomax
Copy link

xfuturomax commented Sep 23, 2020

I've done fix, but it is on review now.

If somebody want to test or use it on different environments before pull-request will be approved and new version will be released with this patch, I've made forks of 3 repositories, you just need to specify it like this in your composer file:

...
"repositories": [      
    {
      "type": "vcs",
      "url": "https://github.com/xfuturomax/framework"
    },
    {
      "type": "vcs",
      "url": "https://github.com/xfuturomax/parser-reflection"
    },
    {
      "type": "vcs",
      "url": "https://github.com/xfuturomax/AspectMock"
    }
  ],
...
"require-dev": {   
    "codeception/aspect-mock": "dev-fix/support-of-new-php-parser-composer"
  },
...

@traceypooh
Copy link

will try to give that a shot now @xfuturomax - thanks!

@traceypooh
Copy link

so far, i keep getting...

    - Installation request for codeception/aspect-mock dev-fix/support-of-new-php-parser-composer -> satisfiable by codeception/aspect-mock[dev-fix/support-of-new-php-parser-composer].
    - codeception/aspect-mock dev-fix/support-of-new-php-parser-composer requires goaop/framework dev-fix/support-of-new-php-parser-composer -> no matching package found.

Potential causes:
 - A typo in the package name
 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://getcomposer.org/doc/04-schema.md#minimum-stability> for more details.
 - It's a private package and you forgot to add a custom repository to find it

Read <https://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.

@xfuturomax
Copy link

@traceypooh Did you specify repositories like in my example above?

@traceypooh
Copy link

i did. but looks like (i'm a bit of a clowncar w/ keyboard keys salad over here with more than basic composer.json...)

but looks like finally got my setup to install like this:

...
    "require-dev": {
        "codeception/aspect-mock": "dev-fix/support-of-new-php-parser-composer",
        "goaop/framework": "dev-fix/support-of-new-php-parser-composer",
        "goaop/parser-reflection": "dev-fix/support-of-new-php-parser",
        "phpunit/phpunit": "*",
        "squizlabs/php_codesniffer": "3.5.6"
    }
...

@traceypooh
Copy link

TESTS WORKING! 🎉 🙏 🙇‍♀️

@troy-rudolph
Copy link

Hello, is there an E.T.A. for a release that contains this solution? Thank you.

1 similar comment
@troy-rudolph
Copy link

Hello, is there an E.T.A. for a release that contains this solution? Thank you.

@Martin1982
Copy link

@Naktibalda it would be very much appreciated if these changes get released

@sollidsnake
Copy link

Looks like this package has been abandoned. Unless somebody forks it, I think this is it, but it would be nice to get a confirmation by the owner and maybe encourage new maintainers.

@Naktibalda
Copy link
Member

@xfuturomax patch to goaop/framework was released in goaop/framework 3.0.0 version on 5th of April. It only supports PHP 7.4.

AspectMock v3 is not compatible with goaop/framework 3.0.0, I will make necessary changes and release AspectMock 4.0.0 supporting goaop/framework v3 only.

@Naktibalda
Copy link
Member

Naktibalda commented Apr 10, 2021

This issue happens in AspectMock's own tests on Github Actions and it happened on Travis last year.

On Github Actions, error happens with PHP 7.0, 7.3 and 7.4, but it doesn't happen with 7.1 and 7.2
I tried to reproduce it locally, but it didn't happen on any PHP versions I had, I tried PHP 7.4, 7.3, 7.2 and 7.0.
I tried to use older versions of nikic/php-parser, goaop/framework and goaop/parser-reflection, but they all worked correctly.

Does anyone know what the issue is and how to fix it? Would adding version constraint for some dependency really help to avoid it?

@traceypooh
Copy link

traceypooh commented Apr 10, 2021

the only fix i've figured out and we're currently using is (above) at

#185 (comment)

we have 2 composer.json files -- one for xenial/php v7.0; one for focal/php v7.4

cat composer.xenial.json 
{
    "require": {
        "ua-parser/uap-php": "3.8.3"
    },
    "require-dev": {
        "codeception/aspect-mock": "3.0.2",
        "nikic/php-parser": "4.5.0",
        "phpunit/phpunit": "6.5.14",
        "squizlabs/php_codesniffer": "*"
    }
}



cat composer.json
{
    "repositories": [
        {
          "type": "vcs",
          "url": "https://traceypooh:xxx@github.com/xfuturomax/framework"
        },
        {
          "type": "vcs",
          "url": "https://traceypooh:xxx@github.com/xfuturomax/parser-reflection"
        },
        {
          "type": "vcs",
          "url": "https://traceypooh:xxx@github.com/xfuturomax/AspectMock"
        }
      ],
    "require": {
        "ua-parser/uap-php": "3.8.3"
    },
    "require-dev": {
        "codeception/aspect-mock": "dev-fix/support-of-new-php-parser-composer",
        "goaop/framework": "dev-fix/support-of-new-php-parser-composer",
        "goaop/parser-reflection": "dev-fix/support-of-new-php-parser",
        "phpunit/phpunit": "*",
        "squizlabs/php_codesniffer": "*",
        "dealerdirect/phpcodesniffer-composer-installer": "^0.7.1",
        "sirbrillig/phpcs-variable-analysis": "^2.10"
    }
}

@traceypooh
Copy link

(and xxx is not my password ;) )

@traceypooh
Copy link

the (best i can tell) character-by-character php-parser parser is broken badly in php v7.4 and mangles files copied into the aspectmock tmp cache dir. so aspectmock pretty much doesnt work in php v7.4

@Naktibalda
Copy link
Member

I created goaop-framework-v3 branch, could you test it on PHP 7.4?

#193

@Naktibalda
Copy link
Member

This issue happened on PHP 7.4 with goaop/framework v2: https://github.com/Codeception/AspectMock/runs/2314193689?check_suite_focus=true

but it doesn't happen with goaop/framework v3, so I consider it fixed. https://github.com/Codeception/AspectMock/pull/193/checks?check_run_id=2364951832

@Naktibalda
Copy link
Member

I released it as 4.0.0 version, please upgrade.

https://github.com/Codeception/AspectMock/releases/tag/4.0.0

@traceypooh
Copy link

working beautifully so far for us over here, so many thanks! 🙏

ramsey added a commit to ramsey/uuid that referenced this issue Sep 23, 2021
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

No branches or pull requests

9 participants