We had an ip_address datatype in D7, but no such type exists for D8.

CommentFileSizeAuthor
#20 3102290-20.patch4.76 KBtr
#20 interdiff-13-20.txt3.28 KBtr
#13 interdiff-6-13.txt4.24 KBtr
#13 3102290-13.patch4.66 KBtr
#6 interdiff.txt3.27 KBdsdeiz
#6 3102290-6.patch4.57 KBdsdeiz
#2 3102290-2.patch3.38 KBdsdeiz

Issue fork typed_data-3102290

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

TR created an issue. See original summary.

dsdeiz’s picture

dsdeiz commented
StatusFileSize
new 3102290-2.patch3.38 KB

Would this happen to work? I'm not sure about the tests though. I've also added the IP address constraint plugin though I guess it might conflict if say core adds the constraint. Mostly copied from TextData.

dsdeiz’s picture

dsdeiz commented
Status: Active » Needs review
tr’s picture

tr commented

Hi @dsdeiz. Thanks for the patch. I think it's a good start. I would like to also have methods to retrieve an IPv4 and IPv6 address from the ip_address data type. I think this is a capability we had in D7.

See for example the \Drupal\Core\TypedData\Plugin\DataType\Timestamp data type for how to do this - this involves defining some methods on the type interface and implementing them in the DataType plugin. Then there should also be tests for creating an ip_address from an IPv4 or an IPv6, and also retrieving an IPv4 or an IPv6 independent of how the ip_address was created.

dsdeiz’s picture

dsdeiz commented
Status: Needs review » Needs work
dsdeiz’s picture

dsdeiz commented
StatusFileSize
new 3102290-6.patch4.57 KB
new interdiff.txt3.27 KB

Yeah, not really sure how this one should be done, sorry. Tried checking the implementation in D7 rules although I'm not finding it.

I wonder if this would work. I just added methods for retrieving and returned NULL if they are not in a valid IP version format depending on the method used.

dsdeiz’s picture

dsdeiz commented
Status: Needs work » Needs review
Anonymous’s picture

Anonymous (not verified) commented

Hello, I have applied this patch and tested it, and I can confirm that this is working for me.

reenaraghavan made their first commit to this issue’s fork.

tr’s picture

tr commented
Version: 8.x-1.x-dev » 2.0.x-dev
sharayurajput’s picture

sharayurajput commented

I reviewed the patch working good looking good to me so we can move this to RTBC

sahilgidwani’s picture

sahilgidwani commented
Status: Needs review » Reviewed & tested by the community
tr’s picture

tr commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3102290-13.patch4.66 KB
new interdiff-6-13.txt4.24 KB

#6 seems fine but it is old and needed to be re-rolled to bring it up to current standards. Here is a new version of that same patch.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/src/Plugin/DataType/IpAddressData.php
@@ -0,0 +1,39 @@
+ *   label = @Translation("Ip address"),

    IP address? 

  2. +++ b/src/Plugin/DataType/IpAddressData.php
@@ -0,0 +1,39 @@
+  public function getIp4Address(): ?string {

+++ b/src/TypedData/Type/IpAddressInterface.php
@@ -0,0 +1,30 @@
+  public function getIp4Address(): ?string;

    getIpV4Address() or getIpv4Address()? getIpv4Address(), + 1 

  3. +++ b/src/Plugin/DataType/IpAddressData.php
@@ -0,0 +1,39 @@
+  public function getIp6Address(): ?string {

+++ b/src/TypedData/Type/IpAddressInterface.php
@@ -0,0 +1,30 @@
+  public function getIp6Address(): ?string;

    getIpV6Address() or getIpv6Address()? getIpv6Address(), + 1 

  4. +++ b/src/Plugin/Validation/Constraint/IpConstraint.php
@@ -0,0 +1,25 @@
+ *   label = @Translation("Ip", context = "Validation"),

    IP?

Otherwise RTBC +1

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Reviewed & tested by the community » Needs review
tr’s picture

tr commented

#1: Thanks, I missed that.
#2, #3: I don't know. Personally I would choose IPv4 and IPv6, because that is the canonical usage. But Drupal is different. I guess Drupal would demand IpV4 and IpV6, but I'm not sure. Do you really think Ipv4 and Ipv6 are better than (IPv4 and IPv6) or (IpV4 and IpV6)?
#4: No. The Symfony validation class is named "Ip" so we're stuck with that.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

#2, #3, I think it depends. If we treat IPv4/IPv6 as a word, then we can go with Ipv4/Ipv6. if we think it's two words -- IP + V4/V6, then it should be IpV4/IpV6,

E.g.
1. LinkedIn, it's a word, not Linked + In,
2. WeChat, it's a word. Not We + Chat.

I would treat IPv4/IPv6 as a word. That's why I prefer Ipv4/Ipv6

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

#4 + * label = @Translation("Ip", context = "Validation"),

>#4: No. The Symfony validation class is named "Ip" so we're stuck with that.

Sorry, it may be unclear. I meant changing Translation("Ip", context = "Validation"), to Translation("IP", context = "Validation"),. Not the class name.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

If an acronym is used in a class or method name, make it CamelCase too (SampleXmlClass, not SampleXMLClass). [Note: this standard was adopted in March 2013, reversing the previous standard.]

In addition to #2, #3, yes, we can not use getIPv4Address/getIPv6Address. See https://www.drupal.org/docs/develop/standards/php/object-oriented-code#n...

tr’s picture

tr commented
StatusFileSize
new interdiff-13-20.txt3.28 KB
new 3102290-20.patch4.76 KB

OK, you have a good argument. I made the changes you suggested. I also added some text to a few documentation comments.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Reviewed & tested by the community

To me, It's ready. @TR, thank you!

  • TR committed ab9067e3 on 2.0.x authored by dsdeiz 
    Issue #3102290 by dsdeiz, jungle, TR: Create an IP Address datatype
tr’s picture

tr commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.