search

amphp / amp

A non-blocking concurrency framework for PHP applications. 🐘
https://amphp.org/amp
MIT License
4.26k stars 257 forks source link

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

Open ghostwriter opened 2 months ago

ghostwriter commented 2 months ago

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.

https://github.com/amphp/amp/blob/138801fb68cfc9c329da8a7b39d01ce7291ee4b0/composer.json#L48-L52

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 commented 2 months ago

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

ghostwriter commented 2 months ago

@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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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