Everything

php

PHP socket ethernet support - step 2

php.internals

David CARLIER

1 year ago

Hi,

I am working on the second pass of this feature and write here because of
the legitimate concern regarding new classes to add.

PR here <https://github.com/php/php-src/pull/17926>

I agree also on adding a namespace on top (Sockets ?) and changing the
class names.

Note that after that I plan at least one more PR (datagram support, reusing
ethernet class as usable one, ...).

Kind regards.

Jakub Zelenka

1 year ago

Hi,

> I am working on the second pass of this feature and write here because of
> the legitimate concern regarding new classes to add.
>
> PR here <https://github.com/php/php-src/pull/17926>
>

For those that want to quickly see the actually classes, here is a stub
link:
https://github.com/php/php-src/pull/17926/files#diff-190e64a9475f39f2b575e6032d6539b504b33fe534965e5bb3512fe430fe1846

> I agree also on adding a namespace on top (Sockets ?) and changing the
> class names.
>

I think it would be good to note that there are already two classes defined
in socket extension:

final class Socket
{
}

final class AddressInfo
{
}

So it might look a bit strange if those two are not namespaced and the new
ones are. The AddressInfo is a bit unfortunate name though. The namespace
seems cleaner but not sure if prefix would be more consistent. Although
having SocketPacket is not exactly nice so probably Socket\Packet is
better. But I don't really mind. I would just really prefer to not
introduce those classes without namespace or prefix.

Regards

Jakub

David CARLIER

1 year ago

On Mon, 3 Mar 2025 at 21:21, Jakub Zelenka <bukka@php.net> wrote:

> Hi,
>
>
>> I am working on the second pass of this feature and write here because of
>> the legitimate concern regarding new classes to add.
>>
>> PR here <https://github.com/php/php-src/pull/17926>
>>
>
> For those that want to quickly see the actually classes, here is a stub
> link:
> https://github.com/php/php-src/pull/17926/files#diff-190e64a9475f39f2b575e6032d6539b504b33fe534965e5bb3512fe430fe1846
>
>
>
>> I agree also on adding a namespace on top (Sockets ?) and changing the
>> class names.
>>
>
> I think it would be good to note that there are already two classes
> defined in socket extension:
>
> final class Socket
> {
> }
>
> final class AddressInfo
> {
> }
>
> So it might look a bit strange if those two are not namespaced and the new
> ones are. The AddressInfo is a bit unfortunate name though. The namespace
> seems cleaner but not sure if prefix would be more consistent. Although
> having SocketPacket is not exactly nice so probably Socket\Packet is
> better. But I don't really mind. I would just really prefer to not
> introduce those classes without namespace or prefix.
>

ACK Sure fair point ; in practice prefixing seems the least disruptive
change.

Gina P. Banyard

1 year ago

On Monday, 3 March 2025 at 21:20, Jakub Zelenka <bukka@php.net> wrote:

> Hi,
>
>> I am working on the second pass of this feature and write here because of the legitimate concern regarding new classes to add.
>>
>> [PR here](https://github.com/php/php-src/pull/17926)
>
> For those that want to quickly see the actually classes, here is a stub link: https://github.com/php/php-src/pull/17926/files#diff-190e64a9475f39f2b575e6032d6539b504b33fe534965e5bb3512fe430fe1846
>
>> I agree also on adding a namespace on top (Sockets ?) and changing the class names.
>
> I think it would be good to note that there are already two classes defined in socket extension:
>
> final class Socket
> {
> }
>
> final class AddressInfo
> {
>
> }
>
> So it might look a bit strange if those two are not namespaced and the new ones are. The AddressInfo is a bit unfortunate name though. The namespace seems cleaner but not sure if prefix would be more consistent. Although having SocketPacket is not exactly nice so probably Socket\Packet is better. But I don't really mind. I would just really prefer to not introduce those classes without namespace or prefix.

Those two classes predate the Namespaces in bundled PHP extensions [1] RFC, I do think adding a namespace is better.
And we could also move the two classes into the new namespace and add class aliases for the global ones, considering they are quite recent additions to PHP.

Best regards,
Gina P. Banyard

[1] https://wiki.php.net/rfc/namespaces_in_bundled_extensions

>

Tim Düsterhus

1 year ago

Hi

On 3/4/25 23:18, Gina P. Banyard wrote:

> Those two classes predate the Namespaces in bundled PHP extensions [1] RFC, I do think adding a namespace is better.
> And we could also move the two classes into the new namespace and add class aliases for the global ones, considering they are quite recent additions to PHP.

I agree that adding a namespace would be appropriate here. Looking at
the sockets.stub.php, it pollutes the global namespace quite a bit. I
probably miscounted, but it appears to be at least 10 different prefixes
for the global constants.

It would probably be in order to also alias all the constants into the
namespace (possibly converting them to enums where appropriate) and then
in a follow-up version deprecate the global ones. I think that would
bring quite a bit of positive impact, for minimal effort.

--------

As for the classes themselves: It does not appear to be defined in our
naming policy [1], but the properties should likely use camel-case, as
that is the established convention for userland code. And I'm also
curious why the port properties are strings, not ints? And what is the
payload object, is that just stdClass?

Best regards
Tim Düsterhus

[1]
https://github.com/php/policies/blob/main/coding-standards-and-naming.rst

David CARLIER

1 year ago

On Tue, 4 Mar 2025 at 22:34, Tim Düsterhus <tim@bastelstu.be> wrote:

> Hi
>
> On 3/4/25 23:18, Gina P. Banyard wrote:
> > Those two classes predate the Namespaces in bundled PHP extensions [1]
> RFC, I do think adding a namespace is better.
> > And we could also move the two classes into the new namespace and add
> class aliases for the global ones, considering they are quite recent
> additions to PHP.
>
> I agree that adding a namespace would be appropriate here. Looking at
> the sockets.stub.php, it pollutes the global namespace quite a bit. I
> probably miscounted, but it appears to be at least 10 different prefixes
> for the global constants.
>

Ok I ll go the namespace route then, fair point.

>
> It would probably be in order to also alias all the constants into the
> namespace (possibly converting them to enums where appropriate) and then
> in a follow-up version deprecate the global ones. I think that would
> bring quite a bit of positive impact, for minimal effort.
>
> --------
>
> As for the classes themselves: It does not appear to be defined in our
> naming policy [1], but the properties should likely use camel-case, as
> that is the established convention for userland code. And I'm also
> curious why the port properties are strings, not ints?

Yes it is a definition mistake it means to be a int indeed.

> And what is the
> payload object, is that just stdClass?
>

Not really it's supposed to be one of the new classes.

Tim Düsterhus

1 year ago

Hi

Am 2025-03-05 13:11, schrieb David CARLIER:

>> And what is the
>> payload object, is that just stdClass?
>>
>
>  Not really it's supposed to be one of the new classes.

Okay, please check if you can find a more appropriate type in the stub
then. `?object` is awfully generic (and thus useless to the user).

Best regards
Tim Düsterhus