{"action":"created","issue":{"url":"https://api.github.com/repos/concrete5/concrete5/issues/3054","labels_url":"https://api.github.com/repos/concrete5/concrete5/issues/3054/labels{/name}","comments_url":"https://api.github.com/repos/concrete5/concrete5/issues/3054/comments","events_url":"https://api.github.com/repos/concrete5/concrete5/issues/3054/events","html_url":"https://github.com/concrete5/concrete5/issues/3054","id":113252366,"number":3054,"title":"Bind fully qualified class names against the application container","user":{"login":"KorvinSzanto","id":1007419,"avatar_url":"https://avatars.githubusercontent.com/u/1007419?v=3","gravatar_id":"","url":"https://api.github.com/users/KorvinSzanto","html_url":"https://github.com/KorvinSzanto","followers_url":"https://api.github.com/users/KorvinSzanto/followers","following_url":"https://api.github.com/users/KorvinSzanto/following{/other_user}","gists_url":"https://api.github.com/users/KorvinSzanto/gists{/gist_id}","starred_url":"https://api.github.com/users/KorvinSzanto/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/KorvinSzanto/subscriptions","organizations_url":"https://api.github.com/users/KorvinSzanto/orgs","repos_url":"https://api.github.com/users/KorvinSzanto/repos","events_url":"https://api.github.com/users/KorvinSzanto/events{/privacy}","received_events_url":"https://api.github.com/users/KorvinSzanto/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-10-25T21:09:07Z","updated_at":"2015-10-26T07:06:14Z","closed_at":null,"body":"## What do you mean?\r\nRight now, we're only binding short string names for all of our reused classes / constructors ('app', 'database', 'session', 'config'). I believe we should start binding the fully qualified class names with their singleton instances and the interfaces with their default implementation.\r\n\r\n## Why should we do this?\r\nBy binding the fully qualified classname, we can use the illuminate container's ability to automatically resolve constructor dependencies.\r\n\r\n### What?\r\nTake this code for example (Copy this into your own `application/bootstrap/app.php` to test):\r\n```php\r\n<?php\r\n// application/bootstrap/app.php\r\n\r\nclass Foo\r\n{\r\n\r\n    public $application;\r\n\r\n    public function __construct(\\Concrete\\Core\\Application\\Application $application)\r\n    {\r\n        $this->application = $application;\r\n    }\r\n\r\n}\r\n\r\n// Get the application instance\r\n$app = \\Core::make('app');\r\n\r\n// Make a Foo and output it.\r\n$foo = $app->make('Foo');\r\ndd($app === $foo->application);\r\n```\r\n\r\nWill this output `true` or `false`? Right now, it will output `false`. \r\nIt's false because we don't pass it the application instance. So then why doesn't it error out? The reason is laravel's container will try to automatically resolve this dependency using the `ReflectionMethod` class. \r\n\r\n### So what does that tell us?\r\nThat tells us we can provide an instance for the class name binding and **automatically provide this dependency to the constructor**! Here's the same example but with the application instance bound to the fully qualified class name:\r\n\r\n```php\r\n<?php\r\n// application/bootstrap/app.php\r\n\r\nclass Foo\r\n{\r\n\r\n    public $application;\r\n\r\n    public function __construct(\\Concrete\\Core\\Application\\Application $application)\r\n    {\r\n        $this->application = $application;\r\n    }\r\n\r\n}\r\n\r\n// Get the application instance\r\n$app = \\Core::make('app');\r\n\r\n// Set the application instance against the fully qualified classname\r\n$app->instance('Concrete\\Core\\Application\\Application', $app);\r\n\r\n// Make a Foo and output it.\r\n$foo = $app->make('Foo');\r\ndd($app === $foo->application);\r\n```\r\n\r\nNow, we're outputting `true`! We have successfully injected our application dependency without having to worry about passing it in. We can do this with every class name that we have.\r\n\r\n## What class names should we provide?\r\nFor `Application`, I'd like to see us bind both `Concrete\\Core\\Application\\Application` and the parent class `Illuminate\\Container\\Container`. Beyond that, we should just start doing this for all of our classes that have singletons floating around and all of our interfaces that have default implementations.\r\n\r\nThoughts?"},"comment":{"url":"https://api.github.com/repos/concrete5/concrete5/issues/comments/151046407","html_url":"https://github.com/concrete5/concrete5/issues/3054#issuecomment-151046407","issue_url":"https://api.github.com/repos/concrete5/concrete5/issues/3054","id":151046407,"user":{"login":"Remo","id":129864,"avatar_url":"https://avatars.githubusercontent.com/u/129864?v=3","gravatar_id":"","url":"https://api.github.com/users/Remo","html_url":"https://github.com/Remo","followers_url":"https://api.github.com/users/Remo/followers","following_url":"https://api.github.com/users/Remo/following{/other_user}","gists_url":"https://api.github.com/users/Remo/gists{/gist_id}","starred_url":"https://api.github.com/users/Remo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Remo/subscriptions","organizations_url":"https://api.github.com/users/Remo/orgs","repos_url":"https://api.github.com/users/Remo/repos","events_url":"https://api.github.com/users/Remo/events{/privacy}","received_events_url":"https://api.github.com/users/Remo/received_events","type":"User","site_admin":false},"created_at":"2015-10-26T07:06:14Z","updated_at":"2015-10-26T07:06:14Z","body":"Spending most of my PHP time with Laravel I definitely support this.\r\n\r\nI never had to do it, but it's possible to override such a class binding from a package, isn't it? I just wonder where I'd have to do that if I wanted to have unit tests for my package and needed to mock something.\r\n\r\nI also wonder if the core would get changed to use that pattern everywhere. Seems like quite an effort, but having it would look pretty nice."}}