[wp-trac] [WordPress Trac] #41083: IP with port number triggers warnings in WP_Community_Events

[WordPress Trac]

#41083: IP with port number triggers warnings in WP_Community_Events
--------------------------------------+-----------------------------
 Reporter:  EatonZ                    |       Owner:  iandunn
     Type:  defect (bug)              |      Status:  assigned
 Priority:  normal                    |   Milestone:  4.9
Component:  Administration            |     Version:  4.8
 Severity:  normal                    |  Resolution:
 Keywords:  good-first-bug has-patch  |     Focuses:  administration
--------------------------------------+-----------------------------

Comment (by iandunn):

 No problem @danieltj, thanks for checking!

 Thanks for pointing that ticket out @birgire, the patch and test data are
 helpful here. I think we'll need the format detection to be a separate
 function from the port-stripping, though, so it can be reused when
 determining the `$netmask`.

 It's tempting to simplify `detect_ip_address_format()` into just
 `$address_format = substr_count( $client_ip, ':' ) > 1 ? 6 : 4` in the
 calling function, but that wouldn't account for invalid input. It's not
 unheard of for things like `'unknown'` to show up in
 `HTTP_X_FORWARDED_FOR` or even `REMOTE_ADDR` (see #41651).

 The `substr_count()` call is better than I came up with, though, because
 it accounts for IPv6 compatibility mode though (e.g.,
 `::ffff:10.20.30.40`). I've updated [attachment:41083.3.diff]  and the
 unit tests to use it.

 I'm not sure about using regex in this case. We gain precision, but it's
 also more fragile and takes extra time for developers working with it to
 read/understand/test/etc. I think they're the best tool when the
 corresponding string-manipulation functions would add lots of extra code
 paths to catch corner cases, etc, but our needs in this case are fairly
 straight-forward. Are there any cases that the `strpos() / substr()`
 approach fails on? If not, then to me it seems like that's simpler and
 more maintainable.

 I'm definitely open to disagreement on that, though. If we do use regex, I
 think we'll need to iterate on those patterns, since they failed some of
 the unit tests. There might be some well-tested, semi-canonical ones
 floating around the web.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/41083#comment:27>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform