[RFC] enum SortDirection

11 days ago by tim@bastelstu.be — view source — reply

unread

please consider this email as a formal request for comments on my modest
proposal of adding a new enum SortDirection to PHP’s standard library.

I have made the full proposal available on the World Wide Web at:

https://wiki.php.net/rfc/sort_direction_enum

Yours sincerely
Tim Düsterhus

11 days ago by Bob Weinand — view source — reply

unread

Hey Tim,

Hi

please consider this email as a formal request for comments on my
modest proposal of adding a new enum SortDirection to PHP’s standard
library.

I have made the full proposal available on the World Wide Web at:

https://wiki.php.net/rfc/sort_direction_enum

Yours sincerely
Tim Düsterhus

That's a good idea, but I wouldn't introduce such class without some
usages at least.

I'd recommend considering immediately upgrading the proposed functions
to use SortDirection and redefine the constants in terms of the enum.

I.e. set const SORT_ASC = SortDirection::Ascending; - similar for others
like the SCANDIR_SORT_DESCENDING etc..

This is a reasonable BC break, which is also trivially fixed by simply
accepting int|SortDirection in existing functions.

Thanks,
Bob

11 days ago by tim@bastelstu.be — view source — reply

unread

That's a good idea, but I wouldn't introduce such class without some
usages at least.

I'd recommend considering immediately upgrading the proposed functions
to use SortDirection and redefine the constants in terms of the enum.

I.e. set const SORT_ASC = SortDirection::Ascending; - similar for others
like the SCANDIR_SORT_DESCENDING etc..

This is a reasonable BC break, which is also trivially fixed by simply
accepting int|SortDirection in existing functions.

A GitHub search for "language:PHP SORT_ASC" reveals that the existing
constants are used in userland code for custom purposes (e.g. within a
switch() statement). Redefining them to the enum would bring little
value, but unnecessarily break them. See also
https://wiki.php.net/rfc/correctly_name_the_rounding_mode_and_make_it_an_enum
for a precedent (specifically the Future Scope section there).

As suggested in the “Future Scope” of the featured RFC widening the core
functions to accept the enum and update the default value to the enum
case can just happen without an RFC, since this should not be observable
except by Reflection.

I want to avoid a situation of explicitly listing some of the
functions in the RFC, missing one and then later having a discussion of
whether or not it requires a follow-up RFC to also adjust that missed one.

Best regards
Tim Düsterhus

11 days ago by Derick Rethans — view source — reply

unread

Hey Tim,

Hi

please consider this email as a formal request for comments on my modest proposal of adding a new enum SortDirection to PHP’s standard library.

I have made the full proposal available on the World Wide Web at:

https://wiki.php.net/rfc/sort_direction_enum

Yours sincerely
Tim Düsterhus

That's a good idea, but I wouldn't introduce such class without some usages at least.

I'd recommend considering immediately upgrading the proposed functions to use SortDirection and redefine the constants in terms of the enum.

I.e. set const SORT_ASC = SortDirection::Ascending; - similar for others like the SCANDIR_SORT_DESCENDING etc..

This is a reasonable BC break, which is also trivially fixed by simply accepting int|SortDirection in existing functions.

If its a BC break, it can't go into a 8.× for something that isn't necessary.

But I also don't see how accepting both int and this new enum for existing built-in functions is a BC break.

Changing the existing constants to something else is probably not a good idea, as other user functions or libraries might have reused them.

cheers
Derick

11 days ago by Larry Garfield — view source — reply

unread

Hey Tim,

Hi

please consider this email as a formal request for comments on my modest proposal of adding a new enum SortDirection to PHP’s standard library.

I have made the full proposal available on the World Wide Web at:

https://wiki.php.net/rfc/sort_direction_enum

Yours sincerely
Tim Düsterhus

That's a good idea, but I wouldn't introduce such class without some usages at least.

I'd recommend considering immediately upgrading the proposed functions to use SortDirection and redefine the constants in terms of the enum.

I.e. set const SORT_ASC = SortDirection::Ascending; - similar for others like the SCANDIR_SORT_DESCENDING etc..

This is a reasonable BC break, which is also trivially fixed by simply accepting int|SortDirection in existing functions.

If its a BC break, it can't go into a 8.× for something that isn't necessary.

But I also don't see how accepting both int and this new enum for
existing built-in functions is a BC break.

Changing the existing constants to something else is probably not a
good idea, as other user functions or libraries might have reused them.

cheers
Derick

I concur. I'd like to see the functions updated now in the RFC, so we get a sense of how many functions will be impacted. Not because I'm concerned about a widened type breaking anything, but more so readers can see the value this brings.

The Yii DB layer (at least in v2, I haven't used v3 yet) used SORT_ASC/SORT_DESC for itself, so I agree that redefining those is unwise.

Nit: Should we use Asc and Desc, for more compact code? Enums are great, but they can get verbose at times. :-) (SortDirection::Descending is 25 characters long, for what is ultimately a named boolean.)

On the whole, though, I'm in favor.

--Larry Garfield

10 days ago by tim@bastelstu.be — view source — reply

unread

I concur. I'd like to see the functions updated now in the RFC, so we get a sense of how many functions will be impacted. Not because I'm concerned about a widened type breaking anything, but more so readers can see the value this brings.

I see the main value in helping userland not to reinvent the wheel every
time and to set some further precedent in including types for this kind
of ubiquitous concept in the standard library.

I don't want to commit to supporting the enum for every possible
function in the standard library for the reasons I mentioned in my reply
to Bob and also because some of the functions already have a very messy
API signature where backfitting the enum is not exactly trivial.

Nit: Should we use Asc and Desc, for more compact code?

I considered using the abbreviations, but then intentionally decided for
the full names. Our naming policy
(https://github.com/php/policies/blob/main/coding-standards-and-naming.rst#acronyms)
states:

Abbreviations and acronyms as well as initialisms SHOULD be avoided wherever possible.

I don't believe that this enum is a case where it is warranted to
diverge from the SHOULD. My rule of thumb here is that for an
abbreviation to be acceptable, it must be the “more common form” of the
term in question.

As an example: I would say “I am sorting the list in descending order by
name” or something like that. However when I am talking about the
protocol that is commonly using TCP port 80 I would say “I am sending a
HTTP request”, not “I am sending a request using the hypertext transfer
protocol”. HTTP is the much much more commonly used term there and thus
using the acronym is preferable. Also note how I used TCP instead of
Transmission Control Protocol there.

Doctrine, as listed in the references section of the RFC, has also opted
to use the full names in their Order enum. Based on this Stack
Overflow answer, using the full name also seems to be more common in the
C# ecosystem: https://stackoverflow.com/a/22963304

Given it's also a strongly typed enum case rather than a “magic string”
such as 'ASC', I expect IDE autocompletion to reduce much of the
typing effort.

Best regards
Tim Düsterhus

1 day ago by Larry Garfield — view source — reply

unread

Hi

I concur. I'd like to see the functions updated now in the RFC, so we get a sense of how many functions will be impacted. Not because I'm concerned about a widened type breaking anything, but more so readers can see the value this brings.

I see the main value in helping userland not to reinvent the wheel every
time and to set some further precedent in including types for this kind
of ubiquitous concept in the standard library.

I don't want to commit to supporting the enum for every possible
function in the standard library for the reasons I mentioned in my reply
to Bob and also because some of the functions already have a very messy
API signature where backfitting the enum is not exactly trivial.

I'm not suggesting we update every single function in one RFC shot. But the RFC should include updating at least one or two so that the Enum is actually used.

Otherwise, there's no real advantage to putting it in core rather than FIG. The reason to put it in core is so it can be used in core. So let's use it in core, even if not the full scope of eventual usage.

--Larry Garfield