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

Namespaced use function declarations aren't prefixed #75

Closed
danieliser opened this issue Oct 15, 2023 · 11 comments
Closed

Namespaced use function declarations aren't prefixed #75

danieliser opened this issue Oct 15, 2023 · 11 comments

Comments

@danieliser
Copy link

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
Copy link

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
Copy link
Author

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

@BrianHenryIE
Copy link
Owner

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
Copy link
Owner

@danieliser That should be fixed in master now: aab2bf6

@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
Copy link
Author

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

@defunctl
Copy link

defunctl commented Nov 1, 2023

@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
Copy link
Owner

@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
Copy link

defunctl commented Nov 1, 2023

@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
Copy link
Owner

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
Copy link

defunctl commented Nov 2, 2023

@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
Copy link
Owner

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

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

No branches or pull requests

3 participants