amp icon indicating copy to clipboard operation
amp copied to clipboard

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

Open ghostwriter opened this issue 1 year ago • 9 comments

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.

ghostwriter avatar Sep 11 '24 14:09 ghostwriter

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

Bilge avatar Sep 11 '24 22:09 Bilge

@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.

ghostwriter avatar Sep 11 '24 23:09 ghostwriter

@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

rela589n avatar Oct 05 '24 07:10 rela589n

@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.

kelunik avatar Oct 05 '24 09:10 kelunik

@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?

rela589n avatar Oct 05 '24 11:10 rela589n

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.

kelunik avatar Oct 05 '24 13:10 kelunik

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


NOTE:

  • We are NOT trying to "just run" Psalm

  • We need the unreleased code on the master branch to implement a specific feature, Psalm needs to be installed and autoloaded for Psalm plugins to work properly.

  • We DO NOT want or need the Psalm.phar

    • https://github.com/psalm/phar/issues/3
    • https://github.com/psalm/phar/issues/14
    • https://github.com/psalm/phar/issues/17
    • https://phpc.social/@ramsey/109752634923665153
    • https://phpc.social/@ramsey/109752751603853019
    • https://phpc.social/@ramsey/109753521483645731

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.

ghostwriter avatar Oct 05 '24 14:10 ghostwriter

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?

trowski avatar Oct 11 '24 03:10 trowski

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 [email protected] and [email protected] (aka psalm@master) in a psalm-plugin package that requires nikic/[email protected].

Notes

ghostwriter avatar Oct 12 '24 13:10 ghostwriter