BrianHenryIE / strauss

Prefix PHP namespaces and classnames to allow multiple versions of libraries to exist without conflict.
https://brianhenryie.github.io/strauss/
MIT License
137 stars 21 forks source link

Namespaced use function declarations aren't prefixed #75

Closed danieliser closed 10 months ago

danieliser commented 11 months ago

Currently any package that declares & uses namespaced functions via the use function ... method, those function's namespaces do not get properly prefixed.

Example package you can try is code-atlantic/chophper on the develop branch: https://github.com/code-atlantic/chophper/tree/develop.

Currently the main branch has them hard coded for each usage which does get properly prefixed and didn't throw errors.

Basically if you have the following in a package, it won't translate and results in fatals.

namespace Chophper; // This works fine.

use function Chophper\some_func; // This doesn't get prefixed properly.
some_func();  // Resulting in errors.

\Chophper\some_func(); // This works fine.
defunctl commented 11 months ago

Just ran into something similar as well,

If you use composer's files autoload, it's broken when using Strauss with namespaced functions, e.g.

{
    "autoload": {
        "files": [
            "functions.php"
        ]
    },
}
danieliser commented 10 months ago

@defunctl we got around it temporarily by not using declared use function ... but rather prefixing each call with the FQN

BrianHenryIE commented 10 months ago

I think this is a duplicate / related of #65 ... both are about namespaced functions.

I have some failing tests written from a when I tried to fix it, but haven't revisited yet: https://github.com/brianhenryie/strauss/compare/master...BrianHenryIE:strauss:issue-65?expand=1

BrianHenryIE commented 10 months ago

@danieliser That should be fixed in master now: https://github.com/BrianHenryIE/strauss/commit/aab2bf675cb0a0dee67ebb8b5a1d852994249ac9

@defunctl I'm not sure what your issue really is. The autoloader type should be unrelated to this issue. Can you check does master fix your problem and if not, open a detailed issue?

danieliser commented 10 months ago

@BrianHenryIE - Awesome, will give it a try later today. Thanks for fixing that for us.

defunctl commented 10 months ago

@BrianHenryIE not sure if it's the same issue, but master still isn't working for me.

Here's a repo detailing the problem, happy to add a new issue if it's not related: https://github.com/defunctl/strauss-test

BrianHenryIE commented 10 months ago

@defunctl Run composer dump-autoload after running Strauss for Composer to reindex the files it loads. After you ran it with "delete_vendor_packages": true it was still looking for /vendor/composer/../ralouphie/getallheaders/src/getallheaders.php.

defunctl commented 10 months ago

@defunctl Run composer dump-autoload after running Strauss for Composer to reindex the files it loads. After you ran it with "delete_vendor_packages": true it was still looking for /vendor/composer/../ralouphie/getallheaders/src/getallheaders.php.

Nice, that worked!

Shouldn't Strauss just do that after it deletes the vendor packages?

Edit: or perhaps just update the docs to include that step?

e.g.

    "scripts": {
        "strauss": [
            "vendor/bin/strauss",
            "@composer dump-autoload"
        ],
BrianHenryIE commented 10 months ago

I'll certainly update the documentation. Strauss should do it, maybe, I'm not sure are there cases where you would not want it. I guess I could make it conditional on the delete_vendor_packages being true and if someone reports it as problematic, I could revert it. I don't use Composer's own autoloader in plugins so haven't thought about this much. I would like to add some command line output as Strauss runs, maybe it could be documented there too.

defunctl commented 10 months ago

@BrianHenryIE let me know if I can help in any way. I could take a stab at improving the documentation, if you're able to tell me a little more about how you personally use it, and if it should be the preferred way to configure Strauss.

Perhaps were all just not using it the most optimal way?

Thanks for all your hard work, it's appreciated.

BrianHenryIE commented 10 months ago

@defunctl TBH, I'm not using it the way I think it should be used! Instead, I'm using it the way I've always been using it.

There are a few open issues related to this, but I think ideally people would download the strauss.phar and after running composer install --no-dev, use it to prefix the contents of /vendor/ without moving/copying anything.

There's at least one related bug right now, where running strauss.phar twice results in double-prefixed namespaces. I haven't looked into fixing it at all yet.

Whenever I do that, I'd like to look at prefixing the uses of prefixed libraries inside existing code. I.e. make adopting Strauss easy for any codebase. Typically, I'm working on new plugins, so I haven't tried this yet. It should be trivial.

I've added a sentence to the README about composer dump-autoload, thanks. https://github.com/BrianHenryIE/strauss/commit/18a7884ded4a139a121a200c8bbcf428158e8ace