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

Build all from dependency injection #88

Merged
merged 5 commits into from
Oct 12, 2015
Merged

Build all from dependency injection #88

merged 5 commits into from
Oct 12, 2015

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Oct 7, 2015

At the moment this solution doesn't work.
This PR describe an idea.

I'm trying to build from DI the event_http_flow but
if I use only DI to build it the application doesn't work
Because ->get('request') and ->get('response') could not be set.

$request = $request ?: ServerRequestFactory::fromGlobals();
$response = $response ?: new Response();
$request = $request ?: $this->getContainer()->get("request");
$response = $response ?: $this->getContainer()->get("response");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single quote

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use $this->container instead of $this->getContainer() for internally call, as I cleaned up on previous PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getContainer is a private API :)

2015-10-08 10:57 GMT+02:00 Abdul Malik Ikhsan notifications@github.com:

In src/App.php
#88 (comment):

  *
  * @return mixed
  */
 public function run($request = null, $response = null)
 {
  •    $request = $request ?: ServerRequestFactory::fromGlobals();
    
  •    $response = $response ?: new Response();
    
  •    $request = $request ?: $this->getContainer()->get("request");
    
  •    $response = $response ?: $this->getContainer()->get("response");
    

use $this->container instead of $this->getContainer() for internally
call, as I cleaned up on previous PR


Reply to this email directly or view it on GitHub
https://github.com/pennyphp/penny/pull/88/files#r41490323.

Gianluca Arbezzano
www.gianarb.it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not, it writtten public function getContainer()

@gianarb
Copy link
Contributor Author

gianarb commented Oct 10, 2015

This implementation creates an inconsistence, if you use $app->run($request, $response) container you will be different values.. but IMO in a limit case..

$app->run($request, $response) is very powerful but during tests

To change request and response in your application now you can override them by DiC.

@fntlnz @EmanueleMinotto @samsonasik feedback around this approach?

@gianarb gianarb closed this Oct 10, 2015
@gianarb gianarb reopened this Oct 10, 2015
$response = $response ?: new Response();

$event = new HttpFlowEvent('bootstrap', $request, $response);
$event = $this->getContainer()->get("http_flow_event");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use single quotes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see https://github.com/pennyphp/penny/blob/master/src/App.php#L48-L51 , getContainer() only return container, and calling internally should not need to call getter, just call $this->container direclty

@gianarb gianarb added this to the 0.6.0 milestone Oct 12, 2015
@gianarb gianarb force-pushed the feature/di-awesome branch from 4e7df8b to 078a1de Compare October 12, 2015 15:37
@gianarb
Copy link
Contributor Author

gianarb commented Oct 12, 2015

Description

With this PR I try to build all by DiC, in my opinion this is a good improvement because now we can replace HttpEventFlow. Before of this improvement I created it into the run method 👎 .

To maintain this feature (powerful into the unit/functional test)

use Penny\App;
$app = new App();
$request = new \Request();
$response = new \Response();

$app->run($request, $response);

I wrote PennyEventInterface common for all http_flow_event implementations, it requires basic functions to execute the run() function.

Gianluca Arbezzano added 4 commits October 12, 2015 22:22
At the moment this solution doesn't work.
This PR rappresent an idea.

I'm trying to build from DI the event_http_flow but
if I use only DI to build it the application doesn't work
Because `->get('request')` and `->get('response')` could not be set.
@gianarb gianarb force-pushed the feature/di-awesome branch from 078a1de to 9c047b3 Compare October 12, 2015 20:49
gianarb pushed a commit that referenced this pull request Oct 12, 2015
Build all from dependency injection
@gianarb gianarb merged commit 4e1b56e into master Oct 12, 2015
@fntlnz
Copy link
Contributor

fntlnz commented Oct 12, 2015

Good job

@gianarb gianarb deleted the feature/di-awesome branch October 13, 2015 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants