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

Allow the compiled container to have a namespace #783

Open
joachim-n opened this issue May 23, 2021 · 4 comments
Open

Allow the compiled container to have a namespace #783

joachim-n opened this issue May 23, 2021 · 4 comments

Comments

@joachim-n
Copy link

If I do:

    $builder->enableCompilation(__DIR__ . '/cache', '\DrupalCodeBuilder\CompiledContainer');

I get:

  [InvalidArgumentException]
  The container cannot be compiled: `\DrupalCodeBuilder\CompiledContainer` is not a valid PHP class name

It should be possible for the container class to be namespaced.

@mnapoli
Copy link
Member

mnapoli commented May 23, 2021

It should be possible for the container class to be namespaced.

Maybe this is a language issue, but this sounds a lot like a demand. Please phrase things a bit better regarding contributors and maintainers.

Ignoring that, could you explain why? What would be the use case?

@joachim-n
Copy link
Author

Do you mean my use of 'should'? I think it's maybe a language issue... I tend to say 'should' in reference to the code itself, e.g. in this bug report I just filed bmewburn/vscode-intelephense#1844. Particularly since bug report prompts usually say 'What do you think should happen?' or 'What do you expect to happen?'. To me, saying 'I expect to be able to use a namespace' definitely sounds like a demand, I guess because it's a statement about myself. Saying 'The code should' feels more abstract to me.

The use case is that I am using php-di in a package which is going to be used with other DI packages. It doesn't feel safe to me to call my compiled container class 'CompiledContainer', because I can't guarantee that another package won't choose that classname too. Using a prefix on the classname like 'MyPackageCompiledContainer' feels a bit PHP 5-ish to me!

@mnapoli
Copy link
Member

mnapoli commented Jun 20, 2021

Particularly since bug report prompts usually say 'What do you think should happen?'

Ah I see, my bad. No harm done anyway, let's move on, thanks for explaining 👍

The use case is that I am using php-di in a package which is going to be used with other DI packages.

OK that use case makes a lot then, thanks for sharing.

I'll be honest: I try to add new features only when strictly necessary. Since the compiled container class is a generated class (that should not appear in autocompletion at development time, even for your end users), "namespacing" using the class name seems perfectly OK to me.

MyPackageCompiledContainer is ugly, I give you that, but it's generated code. A bit like cache data. I don't think it's worth spending time/effort on this.

@dingo-d
Copy link

dingo-d commented Jan 31, 2023

I have a question about this as well. What if our project is using PSR-4 autoloading, and we are in some folder in our project where we define the compilation step? In my case, the compilation will happen in the plugin file inside the src/Core/Plugin.php, and the compiled container will also be inside the src/Core folder.

Now, my Plugin class is namespaced with Vendor/PluginName/Core namespace, but the compiled container is generated without any namespace (just CompiledContainer) which then throws an error when I'm trying to load the class. I tried using file autoloader for that specific file, but that failed as well.

When I manually added namespaces (after the file was compiled), I then got an error that the class cannot be declared, because the name is already in use (in the same place it was declared).

Not really sure how to circumvent this issue 🤷🏼‍♂️

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

No branches or pull requests

3 participants