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

Update class.jetpack.php to improve environment test #1606

Closed
wants to merge 6 commits into from

Conversation

@Cais
Copy link

commented Feb 9, 2015

Adds additional conditional tests for common "local" environments that may have a "." in their site_url() such as ".dev" and ".local"

Update class.jetpack.php to improve environment test
Adds additional conditional tests for common "local" environments that may have a "." in their site_url() such as ".dev" and ".local"
@georgestephanis

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

I think this is a probably a good improvement, but I'd rather see it swapped perhaps with a switch() statement to make it easier to add conditionals -- for example perhaps a filter_var() call, testing the server ip against FILTER_FLAG_NO_PRIV_RANGE and FILTER_FLAG_NO_RES_RANGE (with appropriate tests to handle cases where the server is behind a load balancer and its ip may be an internal ip in one of those ranges)

@Cais

This comment has been minimized.

Copy link
Author

commented Feb 9, 2015

Wow ... um, well, I'm glad it's "probably a good improvement" but you started losing me somewhere after using a switch() and filter_var() call. Those I understand but you might have seen my eyes glazing over as I read the rest (grin). I'll look into these ideas further and see if I can grok what you are suggesting.

I know the patch is not very elegant; more a kluge than anything else ... but I also did not want to let the idea slip away without getting it out there in some functional form.

@georgestephanis

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

Ah yeah, sorry -- was just using the pull request more as an internal scratch pad than anything else.

Thanks!

@Cais

This comment has been minimized.

Copy link
Author

commented Feb 9, 2015

Oh, cool. I'm still going to look at a nicer approach all the same. The switch() idea would make it much easier to extend going forward ...

@Cais

This comment has been minimized.

Copy link
Author

commented Feb 10, 2015

I have a different approach using the switch() function. I'm not seeing how I can change this "patch" so should another "pull request" be made, or is there a better method?

@kraftbj

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

@Cais: To update the PR, just add a commit to the site_url-conditional-update branch that you made originally. Once that's pushed to your fork, it'll automatically update the PR.

+1 to expanding the automatic checks to .dev, .local, and private IP ranges (as noted withe checks re behing LBs).

@Cais

This comment has been minimized.

Copy link
Author

commented Feb 10, 2015

@kraftbj Thanks! I want to tighten up the code a bit more, but I should be able to write the commit to that branch later today ... although I'm still sorting out the "private IP ranges" idea so it may not be there immediately.

@Cais

This comment has been minimized.

Copy link
Author

commented Feb 10, 2015

I still would like to look into the "private IP ranges" but the additional filter hook used in the switch() statement should allow for the idea being externally checked for the time being.

@jeherve jeherve added this to the 3.10 milestone Mar 28, 2016

@samhotchkiss samhotchkiss modified the milestones: 3.10, 4.0.1 Apr 5, 2016

@jeherve jeherve modified the milestones: 4.0.1, 4.0.2, 4.0.3 Apr 21, 2016

@jeherve jeherve modified the milestones: 4.0.3, 4.0.4 May 11, 2016

@samhotchkiss

This comment has been minimized.

Copy link
Contributor

commented May 27, 2016

I think we should only look for URLs that end with .dev or .local-- as is, it would flag "www.devtools.com" or "www.localrestaurants.com" as dev sites...

@samhotchkiss samhotchkiss modified the milestones: 4.1, 4.0.4 May 27, 2016

@jeherve jeherve modified the milestones: 4.3, 4.1 Jun 17, 2016

@richardmuscat richardmuscat modified the milestones: 4.3, 4.4 Jul 7, 2016

@rcoll

This comment has been minimized.

Copy link
Member

commented Aug 29, 2016

I agree with @samhotchkiss. @Cais If you do something like

case ( 0 !== substr_compare( site_url(), '.local', -strlen( '.local' ) ) ) :

instead of

case ( ! strpos( site_url(), '.local' ) ) :

it will detect that .local is only at the end of the site_url().

Additionally, I believe George was recommending the use of filter_var() with some flags to detect if the server's IP address is a local or restricted address.

If you can handle these changes I'd be happy to see what's eligibe for merge in 4.4.

@Cais

This comment has been minimized.

Copy link
Author

commented Aug 29, 2016

@rcoll I'm not sure if/when I will have time to look further into this ticket (at least this week, if not longer ... just too much life going on right now). If this is wanted sooner then please feel free to make the changes as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.