search

hoaproject / Protocol

The Hoa\Protocol library.
https://hoa-project.net/
308 stars 25 forks source link

resolve() polutes global namespace #18

Closed LeonB closed 7 years ago

LeonB commented 7 years ago

Wrapper.php is autoloaded by composer and defines a resolve() function which polutes the global namespace. When running another composer-installed bin which also declares resolve() in the global namespace this gives a conflict. Would it be possible to restrict resolve() to a namespace?

Hywan commented 7 years ago

Hello :-),

Sure we will try to do something. Can you give me something to reproduce please?

LeonB commented 7 years ago

I had the problem when using valet: https://github.com/jmarcher/valet-linux/issues/36

Here resolve() is getting defined in valet: https://github.com/jmarcher/valet-linux/blob/master/cli/includes/helpers.php#L66

I've created a simple test: https://github.com/LeonB/test-issue-18

git clone https://github.com/LeonB/test-issue-18.git
cd test-issue-18/
composer install
php test.php

This outputs:

PHP Warning:  Missing argument 1 for resolve(), called in /home/leon/Public/test/test-issue-18/test.php on line 11 and defined in /home/leon/Public/test/test-issue-18/vendor/hoa/protocol/Wrapper.php on line 599
PHP Notice:  Undefined variable: path in /home/leon/Public/test/test-issue-18/vendor/hoa/protocol/Wrapper.php on line 601
Hywan commented 7 years ago

So you're basically doing the same error than us, and you're complaining 😉? We have 132 direct calls to resolve in all the libraries. This would require a little bit of work. I am willing to do this change, and gently deprecating the resolve alias. However, I don't have time to do this before January. Maybe another Hoacker could do that: @hoaproject/hoackers?

If any one volunteers for this, I can create all the issues.

Hywan commented 7 years ago

Another alternative is to move resolve and other global functions to the closest namespace, require PHP 5.6 as the minimum version, and use use function to import function in the current scope. So it reduces the number of code to migrate.

Pierozi commented 7 years ago

Should we put the alias into Protocol namespace ?

This change mean all our libraries will be updated to major version due to BC ?

Hywan commented 7 years ago

@Pierozi Why all libraries will have BC?

@LeonB After a short discussion, @vonglasow will take the lead on this issue 😃. Thank you!

LeonB commented 7 years ago

@Hywan, I agree my code example is doing the same and isn't the nicest thing to do. But when I've installed hoa/protocol with composer global Wrapper.php is always loaded, even when I'm not directly using it. So another option could be to remove Wrapper.php from the autoload in composer.json.

So in my case, I globally installed phpmetrics/phpmetrics (which has a dependency on hoa/protocol) and another (globally) installed package broke.

Hywan commented 7 years ago

If a library is using hoa/protocol, then it uses Wrapper.php automatically. So, sorry, we cannot remove this file for the prelude.

However, you can rename your function until we remove our resolve function from the global scope. Thoughts? Is it possible?

Pierozi commented 7 years ago

@Hywan Assume your Project use Hoa\Ruler which depend of Hoa\Protocol and your project use resolve if Hoa\Protocol is up to version 2.0 and Ruler up on 2.16.xxx that mean next update of Ruler will break your resolve function no ? and guess require us to update Ruler to 3.0

Hywan commented 7 years ago

@Pierozi Potentially yes. If the BC break of a dependency emerges in the public API, then yes, we should BC break too. That's correct.

Grummfy commented 7 years ago

In fact this function resolve cause issue in laravel app. So it's impossible to use laravel and hoa/ruler together ... or any library depending on protocole. For me, we should assume global vars will be conflictual and prefix them or add them inside a namespace.

HpiGuillaume commented 5 years ago

Hello, I think this issue have to be re-open :/

There is a helper resolve() in .../laravel/framework/src/Illuminate/Foundation/helpers.php who conflict with yours even in a namespace :/

I have the hoa/protocol version "1.17.01.14".

hoa helper: vendor/hoa/protocol/Wrapper.php illuminate helper: vendor/laravel/framework/src/Illuminate/Foundation/helpers.php

The problem don't come from my code but from the package hyn/multi-tenant who use the resolve helper in the Laravel way. You can see it here: vendor/hyn/multi-tenant/src/Providers/Tenants/QueueProvider.php

What can I do to patch it?

mnapoli commented 5 years ago

It seems https://github.com/hoaproject/Protocol/pull/22 was merged, was it supposed to fix this?

To give some context this library is used by Bref via hoa/fastcgi and it conflicts with Laravel's resolve() helper.

splinter89 commented 5 years ago

Codeception also uses this via hoa/console, please release the new version, @Hywan

sanmai commented 5 years ago

As this was "fixed" over a year ago, and isn't released yet, I took liberty to release a forked package with this fix applied:

composer require sanmai/hoa-protocol

Other than that it should be a feature match to 1.17.01.14. Please report any issues.

sanmai commented 4 years ago

@Hywan If there isn't an update planned would you be minding tagging this package as abandoned? This can be done at composer.org. And if you could be so kind to recommend my replacement package, that'd be best.