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

From Twitter: "Cannot use 'Parent' as class name as it is reserved" when running on < PHP 8.0 code #333

Open
dkreuer opened this issue Dec 7, 2021 · 11 comments

Comments

@dkreuer
Copy link
Contributor

dkreuer commented Dec 7, 2021

Source:

Bildschirmfoto 2021-12-07 um 18 15 01

Is it possible to check non-PHP8 code using PHAR? Because I've tried latest release and I got "Cannot use 'Parent' as class name as it is reserved". I've created issue some time ago to implement this tool (we had soft dependencies) but we still did not manage to do it. -- https://twitter.com/Wirone

Going to analyse if it's happening with PHP 7.4.* or only with PHP ^8.0.

@Ocramius
Copy link
Collaborator

Ocramius commented Dec 7, 2021

@Wirone do you have some source codebase (or code snippet) where this happens, perhaps?

@dkreuer
Copy link
Contributor Author

dkreuer commented Dec 7, 2021

PHP 7.4.6 (lowest I have atm) throws fatal error:

PHP Fatal error: Cannot use 'Parent' as class name as it is reserved in test/fixtures/regression/CannotUseParentAsClassNameAsItIsReservedPhp7Test/src/Parent.php on line 7

All supported versions yield the same result: https://3v4l.org/11OAR

Without additional information I reckon it's "cannot reproduce".

@Wirone
Copy link

Wirone commented Dec 7, 2021

@Ocramius sorry for being late but I tweeted to you from mobile and for few hours I did not use computer. Here's reproducer:

{
    "name": "maglnet/composer-require-checker-reproducer",
    "description": "Reproducer for \"Cannot use 'Parent' as class name as it is reserved\"",
    "type": "project",
    "require": {
        "quickbooks/v3-php-sdk": "^6.0"
    },
    "minimum-stability": "stable"
}

Running composer-require-checker check results with:

❯ composer-require-checker check
ComposerRequireChecker 3.8.0@537138b833ab0f9ad72b667a72bece2a765e88ab

In LocateASTFromFiles.php line 46:

  Parsing the file [/Volumes/Projects/RND/composer-require-checker/vendor/quickbooks/v3-php-sdk/src/XSD2PHP/test/data/expected/Xsd2Php/MavenTests/bindings/org/apache/maven/POM/_4_0_0/Parent.php] resulted in an error: Cannot use 'Parent' as class name as it is reserved on line 11

In ParserAbstract.php line 919:

  Cannot use 'Parent' as class name as it is reserved on line 11

Tested on PHP 7.4 and 8.1 (both with CRC 3.8.0).

@dkreuer
Copy link
Contributor Author

dkreuer commented Dec 7, 2021

Running find . -type f -name '*.php' -exec php -l {} \; |grep -v "No syntax errors detected" on the dependency codebase yielded the following errors (with PHP 7.4.6):

PHP Deprecated:  __autoload() is deprecated, use spl_autoload_register() instead in ./src/XSD2PHP/test/Bootstrap.php on line 8

Deprecated: __autoload() is deprecated, use spl_autoload_register() instead in ./src/XSD2PHP/test/Bootstrap.php on line 8

PHP Fatal error:  Cannot use 'Parent' as class name as it is reserved in ./src/XSD2PHP/test/data/expected/Xsd2Php/MavenTests/bindings/org/apache/maven/POM/_4_0_0/Parent.php on line 11

Fatal error: Cannot use 'Parent' as class name as it is reserved in ./src/XSD2PHP/test/data/expected/Xsd2Php/MavenTests/bindings/org/apache/maven/POM/_4_0_0/Parent.php on line 11

Errors parsing ./src/XSD2PHP/test/data/expected/Xsd2Php/MavenTests/bindings/org/apache/maven/POM/_4_0_0/Parent.php

That's nothing we can change since PHP 7.x defines the reserved keyword parent.

There are "test" files placed under "src" and therefore these are parsed by composer-require-checker.

There is currently no way to exclude paths in vendors from parsing. And there should not be given the fact that the internal structure of a dependency is nothing the using project should care about.

In the dependency itself it might be possible to exclude the tests via autoload > exclude-from-classmap but I could not make it work in 30 minutes testing time. Better to move the XSD2PHP dependency from src directory to sth. like lib and reference it from there. I suggest to approach the maintainers of quickbooks/v3-php-sdk to restructure their repository and fix the problem there.

@Ocramius Do you have pleas for having a configurable blacklist of paths in vendor not to consider while analysing a project? It would solve the actual problem without interacting with upstream but would open up a whole new category of issues "why is the symbol not found, it's clearly defined -- because the file defining the symbol has been blacklisted" - and the blacklist if using patterns might interfere with dependency updates ... headache included IMHO.

@Ocramius
Copy link
Collaborator

Ocramius commented Dec 8, 2021

Oh, wow, that QuickBooks stuff includes a whole test suite (and foreign libraries) under its autoload config: https://github.com/intuit/QuickBooks-V3-PHP-SDK/tree/v5.4.2/src/

My suggestion is to report an upstream bug: this stuff should at least be in their export-ignore.

Other folks seem to have similar problems with it:

/cc @abisalehalliprasan

@Ocramius
Copy link
Collaborator

Ocramius commented Dec 8, 2021

you have pleas for having a configurable blacklist of paths in vendor not to consider while analysing a project? It would solve the actual problem without interacting with upstream but would open up a whole new category of issues "why is the symbol not found, it's clearly defined -- because the file defining the symbol has been blacklisted" - and the blacklist if using patterns might interfere with dependency updates ... headache included IMHO.

I think configuration is not the problem here: the tool is highlighting a problem, which is real (and part of a broken dependency).

Adding configuration to shadow/exclude (let's avoid "blacklist") certain paths from a scan is possible in future, but it's not at the core of the problem here.

@Wirone
Copy link

Wirone commented Dec 8, 2021

@dkreuer @Ocramius thanks for taking your time on it and your suggestions. Looking at PR mentioned above I think there's really small chance that quickbooks/v3-php-sdk's maintainer would do anything about it. More than a year since opening 😱 I'll look at our codebase and verify if this package is actually used - FYI: it's huge monolith app with large legacy part (many years of development), there's 314 Composer dependencies right now and it's sometimes not easy to work with it 😓

@Ocramius
Copy link
Collaborator

Ocramius commented Dec 8, 2021

@Wirone the maintainer of that library ( @abisalehalliprasan ) is mentioned/pinged in this issue :-)

@Wirone
Copy link

Wirone commented Dec 8, 2021

@Ocramius I think we need to migrate from quickbooks/v3-php-sdk to quickbooks/payments-sdk, I've created internal issue for that. But I don't know when it could be done so it would be really great if this could be fixed somehow - here or in Quickbooks package 🙂 Because as of now I can't even analyse current dependencies.

@dkreuer
Copy link
Contributor Author

dkreuer commented Dec 13, 2021

@Ocramius exclude feature to circumvent upstream packages issues? Pro: require checker will be usable in these cases too. Con: More configuration which might go wrong.

I’d put that feature on my todo list for OSS work in January if it‘s greenlit.

@dkreuer
Copy link
Contributor Author

dkreuer commented Dec 13, 2021

Issue and solution is probably a duplicate of #58. and there already was doubt if such an option would be worth it.

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

No branches or pull requests

3 participants