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

Function Redeclaration in */functions.php Due to Missing Safety Guards #443

Open
ghostwriter opened this issue Sep 11, 2024 · 9 comments
Open

Comments

@ghostwriter
Copy link

Hi,

I'm experiencing an issue with a tool that relies on this package.

The problem occurs when the tool tries to include vendor/autoload.php which automatically includes these functions via composer.

amp/composer.json

Lines 48 to 52 in 138801f

"files": [
"src/functions.php",
"src/Future/functions.php",
"src/Internal/functions.php"
]

It results in a "previously declared" function error due to a lack of safety guards to check if the function already exists.

Here is a screenshot illustrating the issue:

image

Suggested Fix:

Add a safety guard to the functions to prevent redeclaration, like:

if (! \function_exists('functionName')) {
    // function definition here
}

Thank you for your time.

@Bilge
Copy link
Contributor

Bilge commented Sep 11, 2024

How do you wilfully end up with two definitions for \Amp\delay, and why do you believe it's correct to do so?

@ghostwriter
Copy link
Author

@Bilge I was using "master" branch of psalm, (globally installed via composer because i need the new version of php-parser for a psalm plugin).

Composer is autoloading the global autoload file , ~/.composer/vendor/autoload.php and local autoload file ~/Desktop/oss/ghostwriter/atprotocol/vendor/autoload.php.

If the guard was there, it would work without any issues.

@rela589n
Copy link

rela589n commented Oct 5, 2024

@ghostwriter , I also faced this same issue when using psalm. From my side, the solution was moving it into vendor-bin (by means of composer bin plugin) and installing it there. Also, there (in vendor-bin) I patched (composer patches) amp library in order to not re-declare function if it was already declared. Finally, I had to define custom boostrap.php file to first include my main project autoload.php file and only then autoload of vendor-bin directory.

See https://github.com/rela589n/knowledge-base/blob/main/PHP/Psalm%20in%20a%20separate%20composer.json.md

@kelunik
Copy link
Member

kelunik commented Oct 5, 2024

@rela589n It very much sounds like you're fighting the symptom instead of the root cause.

Two different versions of the library might lead to issues, adding the guard would just hide the symptom.

I highly recommend to use the psalm phar, like all amphp projects do.

@rela589n
Copy link

rela589n commented Oct 5, 2024

@kelunik , as far as I remember I could not have managed to get it working correctly when running from phar either because of this same problem of already declared function.

Maybe you have tried doing this with the different outcome?

@kelunik
Copy link
Member

kelunik commented Oct 5, 2024

All our packages use Psalm and we've never had this problem. You can have a look at the CI config for GitHub actions to compare it to your setup.

@ghostwriter
Copy link
Author

Hey @rela589n Thank you, we will try your solution. 🙏🏾 I appreciate you not being dismissive and condescending. ❤️


NOTE:

Unrelated: I had previously attempted to use the Psalm Phar but encountered issues, particularly because the version compatible with PHP-Parser v5 only exists on the master branch.

@trowski
Copy link
Member

trowski commented Oct 11, 2024

I'm confused as to how Psalm 5.x was installed along side Amp 3.x. Composer should not be allowing this, since Psalm 5.x requires Amp 2.x.

Could you explain more about your set up and how there are seemingly two versions of Amp installed simultaneously?

@ghostwriter
Copy link
Author

I’ve resolved this issue for myself using ghostwriter/handrail.

Special thanks to @rela589n for the inspiration.

Feel free to keep this issue open if you’d like to explore further.

I’ll be unsubscribing as I’m no longer being blocked by this issue.

Thanks!


Goal

Integrate phpunit@11.x and psalm@6.x (aka psalm@master) in a psalm-plugin package that requires nikic/php-parser@5.x.

Notes

  • Version Compatibility:

    • psalm@6.x (aka psalm@master) requires amphp/amp version 3.x.
    • psalm@5.x requires amphp/amp version 2.x.
  • Dependencies:

    • psalm@6.x (aka psalm@master) has a direct dependency on nikic/php-parser version 5.x.
    • psalm@5.x has a direct dependency on nikic/php-parser version 4.x.
    • phpunit@11.x indirectly depends on nikic/php-parser version 5.x through phpunit/php-code-coverage.

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

No branches or pull requests

5 participants