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

[5.8] Validate container alias registration #27390

Merged
merged 1 commit into from Feb 4, 2019
Merged

[5.8] Validate container alias registration #27390

merged 1 commit into from Feb 4, 2019

Conversation

seriquynh
Copy link
Contributor

The Container::getAlias() method currently throws LogicException. This exception is forwarded to many other container methods that call getAlias() and there are a lot of conflicts with the container interface.

  • \Illuminate\Container\Container::when() >>> breaks container interface.
  • \Illuminate\Container\Container::resolved() >>> breaks container interface.
  • \Illuminate\Container\Container::addContextualBinding() >>> breaks container interface
  • \Illuminate\Container\Container::extend() >>> breaks container interface
  • \Illuminate\Container\Container::rebinding()
  • \Illuminate\Container\Container::resolve()
  • \Illuminate\Container\Container::resolving() >>> breaks container interface
  • \Illuminate\Container\Container::afterResolving() >>> breaks container interface
  • \Illuminate\Container\Container::getExtenders()
  • \Illuminate\Container\Container::forgetExtenders()

Let's validate container alias registration to avoid these conflicts. And I think it's reasonable to add @throws \LogicException to \Illuminate\Contracts\Container\Container::alias(). Each class has to implement this logic.

@seriquynh seriquynh changed the title [5.8] Container validates alias on registration [5.8] Validate container alias registration Feb 2, 2019
@imanghafoori1
Copy link
Contributor

Maybe the message at this phase should be changed into a more meaningful one.
like:

"[{$abstract}] cannot be aliased to itself.

@seriquynh
Copy link
Contributor Author

This message is fine now and I just focus on the idea. I think if this is accepted, you or someone can change the message.

@imanghafoori1
Copy link
Contributor

Taylor usually makes these modifications himself after merging. I just mention them, as a note.
Hoshimi's son.

@seriquynh
Copy link
Contributor Author

I know. So, let him decide. Thanks!

@driesvints
Copy link
Member

I agree with this change but we should definitely document this in the upgrade guide.

@taylorotwell taylorotwell merged commit dfc469b into laravel:master Feb 4, 2019
@seriquynh seriquynh deleted the validate_container_alias_registration branch February 5, 2019 07:19
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.

None yet

4 participants