Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Cache ipCheck (2.8)#22819

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
gonzalovilaseca wants to merge1 commit intosymfony:2.8fromgonzalovilaseca:patch-2
Closed

Cache ipCheck (2.8)#22819

gonzalovilaseca wants to merge1 commit intosymfony:2.8fromgonzalovilaseca:patch-2

Conversation

@gonzalovilaseca
Copy link
Contributor

@gonzalovilasecagonzalovilaseca commentedMay 21, 2017•
edited
Loading

In our app we use trusted proxies. Using Blackfire we foundIpUtils::checkIp was being called 454 times taking 3.15ms.
Caching the result saves those 3ms.

QA
Branch?2.8
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

*/
protected$defaultLocale ='en';

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to bebool, see fabbot.io patch

returnfalse;
}

if (!isset($this->validTrustedProxyIP)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible thatself::$trustedProxies is changed between method calls. Make sure to reset the cached result insetTrustedProxies and that this is tested.

@nicolas-grekas
Copy link
Member

shouldnt this fix be submitted against the 2.8 branch?

gonzalovilaseca and apetitpa reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the2.8 milestoneMay 21, 2017
@tgalopin
Copy link
Contributor

tgalopin commentedJun 4, 2017•
edited
Loading

I stumbled upon the same issue. Shouldn't the cache be stored statically inIpUtils instead of in the request? It makes more sense to me, as the request is a representation of a HTTP object, using it to store cache feels wrong. Moreover, using a cache in IpUtils ensure a usage outside of the Request would still be cached.

WDYT?

@fabpot
Copy link
Member

Indeed, a cache inIpUtils seems a better idea.

@tgalopin
Copy link
Contributor

@gonzalovilaseca do you want to do it? I can if you don't have time right now.

$request =newRequest(array(),array(),array(),array(),array(),array('REMOTE_ADDR' =>'8.8.8.8'));
Request::setTrustedProxies(array('8.8.8.8'), Request::HEADER_FORWARDED);

$this->assertEquals(true,$request->isFromTrustedProxy());
Copy link
Contributor

@robfrawleyrobfrawleyJun 4, 2017•
edited
Loading

Choose a reason for hiding this comment

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

$this->assertTrue($this->isFromTrustedProxy());

Also seen below intestIsFromTrustedProxyCacheIsInValidatedOnSetTrustedProxies().

@gonzalovilaseca
Copy link
ContributorAuthor

@tgalopin Sure, I'll do it, will be ready by tuesday

tgalopin reacted with thumbs up emoji


Request::setTrustedProxies(array('5.5.5.5'), Request::HEADER_FORWARDED);

$this->assertEquals(false,$request->isFromTrustedProxy());
Copy link
Contributor

Choose a reason for hiding this comment

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

assertFalse()

publicfunctiontestIsFromTrustedProxyReturnsFalseIfNoTrustedProxies()
{
$request =newRequest();
$this->assertEquals(false,$request->isFromTrustedProxy());
Copy link
Contributor

Choose a reason for hiding this comment

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

assertFalse()

}

$remoteAddr =$this->server->get('REMOTE_ADDR');
if (!array_key_exists($remoteAddr,self::$validTrustedProxyIP) ||null ===self::$validTrustedProxyIP[$remoteAddr]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simply:!isset(self::$validTrustedProxyIP[$remoteAddr]) cause you do check fornull anyway.

@gonzalovilaseca
Copy link
ContributorAuthor

I've updated the PR moving the cache toIpUtils, I think that anIpUtils decorator would be a better solution, but being called statically from other places it would mean a bit of refactoring, should I go thedecorator way or is this ok?

Feel free to improve the naming of the new methods, naming is not my strength, even less with jet lag :-).

{
/**
* @var array
*/
Copy link
Member

Choose a reason for hiding this comment

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

Should be private

* @return bool Whether the request IP matches the IP, or whether the request IP is within the CIDR subnet
*/
publicstaticfunctioncheckIp4($requestIp,$ip)
{
Copy link
Member

Choose a reason for hiding this comment

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

I would not delegate to another method but just copy/paste the cache logic in the methods directly.

publicstaticfunctioncheckIp4($requestIp,$ip)
{
$cacheKey =$requestIp.'-'.$ip;
if (!array_key_exists($cacheKey,self::$checkedIps)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would inline the checkIp4Internal method as well.

class IpUtils
{
/**
* @var array
Copy link
Member

Choose a reason for hiding this comment

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

phpdoc should be removed

returnfalse;
}
$cacheKey =$requestIp.'-'.$ip;
if (!array_key_exists($cacheKey,self::$checkedIps)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would return early:

if (array_key_exists($cacheKey,self::$checkedIps)) {returnself::$checkedIps[$cacheKey];}

I would even changearra_key_exists() toisset()

publicstaticfunctioncheckIp4($requestIp,$ip)
{
$cacheKey =$requestIp.'-'.$ip;
if (isset($cacheKey,self::$checkedIps)) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks wrong to me

@fabpot
Copy link
Member

@gonzalovilaseca Can you change the branch of the PR to 2.7?

@gonzalovilaseca
Copy link
ContributorAuthor

@fabpot 2.7 or 2.8? (nicolas mentioned 2.8)

@gonzalovilasecagonzalovilaseca changed the base branch frommaster to2.7June 6, 2017 19:23
@fabpot
Copy link
Member

I would say 2.7.

@gonzalovilasecagonzalovilaseca changed the base branch from2.7 to2.8June 6, 2017 19:25
@gonzalovilasecagonzalovilaseca changed the base branch from2.8 to2.7June 6, 2017 19:26
@gonzalovilaseca
Copy link
ContributorAuthor

@fabpotIpUtils changed from 2.7 to 2.8, should I do a PR for each branch?

@fabpot
Copy link
Member

That would be wonderful, thanks.

@gonzalovilasecagonzalovilaseca changed the base branch from2.7 to2.8June 7, 2017 19:13
@gonzalovilasecagonzalovilaseca mentioned this pull requestJun 7, 2017
@gonzalovilaseca
Copy link
ContributorAuthor

2.7 version here:#23098

@gonzalovilasecagonzalovilaseca changed the titleCache ipCheckCache ipCheck (2.8)Jun 7, 2017
@fabpotfabpot closed thisJun 8, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot requested changes

+3 more reviewers

@robfrawleyrobfrawleyrobfrawley left review comments

@stloydstloydstloyd requested changes

@sstoksstoksstok requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

2.8

Development

Successfully merging this pull request may close these issues.

8 participants

@gonzalovilaseca@nicolas-grekas@tgalopin@fabpot@stloyd@sstok@robfrawley@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp