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

using HTTP\Request instance to pull ip address instead of $_SERVER['REMOTE_ADDR'] #1180

Merged

Conversation

samsonasik
Copy link
Member

No description provided.

@puschie286
Copy link
Contributor

puschie286 commented Sep 7, 2018

i guess its not necessary to create a new config instance each time -> would use config( App::class ) for the global instance. I also think the same should be possible for HTTP/Request instances or are there reasons not to do so?

@samsonasik
Copy link
Member Author

using config(App::class) will require to define at App class property, while it is calling function inside HTTP\Request class. The another solution may add new constant inside application/Config/Boot/* or application/Constants.php

@puschie286
Copy link
Contributor

? its the same as you do, but no new instance ( ok you will have to do config( \Config\App::class ), sry for forgetting namespace )

@samsonasik
Copy link
Member Author

What I meant is, using new instance in class property is not doable in php, eg;

class App
{
    public $ip = (new Request())->getIpAddress();
}

The alternative is by define as constant

@lonnieezell
Copy link
Member

It's better to use a service for that instead of creating a new instance every time:

Services::request()->getIpAddress()

Or, better yet, have the Request object passed into the class' constructor and saved as a class property....

@samsonasik samsonasik force-pushed the using-request-to-pull-ipaddresss branch from 08863a6 to 18161d2 Compare September 7, 2018 14:22
@samsonasik
Copy link
Member Author

I've updated to set ipAddress property in __construct() in each of the class that require it.

@jim-parry jim-parry added this to the 4.0.1? milestone Sep 27, 2018
@jim-parry
Copy link
Contributor

@lonnieezell I am good with this. Ok to merge?

@lonnieezell
Copy link
Member

That's not exactly what I meant. The preferred way to do this is to grab a copy of the IP address in the services class, where the driver is instantiated and pass it in the constructor of the driver.

@samsonasik samsonasik force-pushed the using-request-to-pull-ipaddresss branch from dc57094 to d063916 Compare October 29, 2018 17:59
@samsonasik
Copy link
Member Author

@lonnieezell implemented

@samsonasik
Copy link
Member Author

@samsonasik samsonasik force-pushed the using-request-to-pull-ipaddresss branch from a6a4be9 to 5192e31 Compare October 29, 2018 18:58
@samsonasik
Copy link
Member Author

as all handlers has ipAddress property, I add ipAddress property to BaseHandler with inject in 2nd parameter.

@lonnieezell
Copy link
Member

Looks good. Thanks!

@lonnieezell lonnieezell merged commit 32716de into codeigniter4:develop Oct 30, 2018
@samsonasik samsonasik deleted the using-request-to-pull-ipaddresss branch October 30, 2018 05:05
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