Allow unbound class resolution from container

[girardinsamuel]

Fix #618.

[girardinsamuel]

@os-nikita I reopened your PR here.

@josephmancuso What do you think of this ? I reviewed it, it looks good to me 👌. The only thing is: what happens when we try to resolve somehting into the container which really does not exist ?

[os-nikita]

The only thing is: what happens when we try to resolve somehting into the container which really does not exist ?

@girardinsamuel In which scenario do you envision that would happen? 🤔

[girardinsamuel]

Yes, I am not sure that this could happen. But somehow if the class is not imported correctly and we try to resolve it, will the error be caught correctly ?

[os-nikita]

Let me do a few tests to confirm later today.

[josephmancuso]

@os-nikita I reopened your PR here.

@josephmancuso What do you think of this ? I reviewed it, it looks good to me 👌. The only thing is: what happens when we try to resolve somehting into the container which really does not exist ?

@girardinsamuel I'm not sure what you mean here. What do you mean by class does not exist? If a class is not imported correctly, won't you get an import error?

And if you do import a class that's not right ... I am not sure I understand the issue lol

[os-nikita]

Yep that's my conclusion here as well. An import error will happen regardless of this PR, it does not do anything magical to get around that.

[os-nikita]

Perhaps the only thing I may add/adjust is the way the application/container itself is resolved using this approach. 🤔

[girardinsamuel]

You're right, should not be an issue finally..

add/adjust is the way the application/container itself is resolved using this approach.

What do you mean here ?

[os-nikita]

If someone adds foundation.Application or container.Container as a dependency in a method.

[girardinsamuel]

If someone adds foundation.Application or container.Container as a dependency in a method.

Oh yes, well it's not a good idea to do this. Do you want to add a safeguard for this ?

[josephmancuso]

If someone adds foundation.Application or container.Container as a dependency in a method.

I'll have to test this use case but i believe the container binds itself to the container so that would work and resolve fine. The application class maybe not but i would think we can just bind the application class to the container as well.

On another note, if there are dependencies that are not in the container it would throw an error like "Cannot resolve parameter application: Application"

[os-nikita]

Something like this?

os-nikita@7c14c4c

[os-nikita]

On another note, if there are dependencies that are not in the container it would throw an error like "Cannot resolve parameter application: Application"

Did you mention this related to the container/application self-resolution discussion? Or in general? Because this PR was crafted especially to allow dependencies to be resolved automatically within method arguments e.g., in controllers. So the above error would no longer appear as long as the dependency can be correctly constructed. I added tests to demonstrate this using nested dependencies. Perhaps adding another test to see it in action with a controller would be clearer.

[girardinsamuel]

No that's okay for me 😉. I am not sure about the last one os-nikita@7c14c4c though...else it's good to me !

@josephmancuso can you do a final review ?

[josephmancuso]

On another note, if there are dependencies that are not in the container it would throw an error like "Cannot resolve parameter application: Application"

Did you mention this related to the container/application self-resolution discussion? Or in general? Because this PR was crafted especially to allow dependencies to be resolved automatically within method arguments e.g., in controllers. So the above error would no longer appear as long as the dependency can be correctly constructed. I added tests to demonstrate this using nested dependencies. Perhaps adding another test to see it in action with a controller would be clearer.

Yeah you are right.

[josephmancuso]

Merged in. PR looks great 👍 awesome job

[girardinsamuel]

Because of type hinting application with "Application" it looks like it give some issues now we are trying to resolve any classes...

[os-nikita]

@girardinsamuel What do you mean? There are issues now that this has been merged in? 🤔

[josephmancuso]

@girardinsamuel What do you mean? There are issues now that this has been merged in? 🤔

I tested this on a test app and everything looked great but we think there could be some conflicts with local development having conflicts between the namespacing (src.masonite.response.Response vs masonite.response.Response)

[josephmancuso]

we're still investigating

[os-nikita]

@girardinsamuel What do you mean? There are issues now that this has been merged in? 🤔

I tested this on a test app and everything looked great but we think there could be some conflicts with local development having conflicts between the namespacing (src.masonite.response.Response vs masonite.response.Response)

I did see this at some point during testing when trying out the "self-resolving container" approach and was confused by this namespace difference.

[girardinsamuel]

Here is a PR adding more tests to container to demonstrate a potential issue.
#637
The two last tests are failing demonstrating issue that can arise from parameter type hinted with strings.

Note that the namespaced issue is another issue which is independant from the changes of this PR.