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

[6.x] Fixed bad dependency assumptions #31894

Merged
merged 1 commit into from
Mar 11, 2020
Merged

[6.x] Fixed bad dependency assumptions #31894

merged 1 commit into from
Mar 11, 2020

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Mar 10, 2020

  1. The scheduler uses the native container (and not the contract), and so we must ensure that it is recommended to users both by composer, and by friendly runtime exceptions. If we wanna refactor it to use the contract, that is a job for the master branch.
  2. We should not be calling stuff from the foundation helper functions file. We should instead be resolving the jobs and the job dispatcher from the container, and providing good developer experience if we fail to resolve it. Removal of the calls to the foundation functions also made it clear we actually had an dependency on illuminate/queue too, for packing Closures.

Closes #31891.

@nmokkenstorm
Copy link
Contributor

nmokkenstorm commented Mar 10, 2020

I opened the issue to start a disccusion, but this works too. Wouldn't injecting an implementation of Illuminate\Contracts\Container\Container be way easier instead of dynamically checking if an instance can be resolved? Right now you're adding illuminate\container as an optional dependency, but in reality this whole class doesn't work without it being available.

This is by no means meant as critique, it just doesn't match the approach I had in mind, so I'm trying to follow your thought process here.

Also please let me know if I can be of any help in this, I was originally planning on opening a PR after gathering some thoughts on the issue, but I'm more than happy to help out here instead.

@GrahamCampbell
Copy link
Member Author

I opened the issue to start a disccusion, but this works too. Wouldn't injecting an implementation of Illuminate\Contracts\Container\Container be way easier instead of dynamically checking if an instance can be resolved?

We have no means of doing that without having a container, and even if we could, as I said, that would fall out of the remit of a bug fix. Such a refactoring would have to target the master branch.

@GrahamCampbell GrahamCampbell changed the title [6.x] Fixed bad dependency assumptions [WIP] [6.x] Fixed bad dependency assumptions Mar 10, 2020
@GrahamCampbell GrahamCampbell changed the title [WIP] [6.x] Fixed bad dependency assumptions [6.x] Fixed bad dependency assumptions Mar 11, 2020
@taylorotwell taylorotwell merged commit cf71f81 into 6.x Mar 11, 2020
@GrahamCampbell GrahamCampbell deleted the dep-fixes branch March 11, 2020 18:40
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

Successfully merging this pull request may close these issues.

4 participants