WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 7 days ago

#51278 reviewing enhancement

Update return types to reflect the real return types. Remove mixed.

Reported by: ReneHermi Owned by:
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: docs, coding-standards Cc:

Description

In the DocBlocks, we document return values with mixed only.

That is no good practice as it does not show the real available and possible return types.
A better approach is to declare and document explicitly return values separated by a pipe character like this @return bool|string

That is much clearer because developers do not need to check the return values by reading the entire docBlock, reading the whole code of a method, or reading the (lengthy) function description under developer.wordpress.org.

A simplemixed is outdated and makes it hard for every severe developer to start with coding for WordPress as he has to continually check the expected return values when there is a mixed.

As a bonus, the documentation under developer.wordpress.org will be updated as well by this change automatically. It will help developers getting know the correct return values immediately from the docs without reading all the extra special explanations that we added to our docs whenever we use mixed.

That is a huge time saver and makes it much faster for every developer to write solid and better code faster.

If this is accepted for core, I will offer to work on this on all other methods as well.

That will be a long-lasting process due to the number of functions, but it's definitely worth the effort. We should make this change to all methods where we usemixed and make it to our daily little take-care moment when we add new methods to WordPress.

Sample Patch by using get_option() as an example for all methods that use mixed:
https://github.com/WordPress/wordpress-develop/pull/523

get_option() is a special case, though as it automatically unserializes. So it supports all non-scalar types. Most other methods in WordPress uses mixed with only two return types like bool|string representation, which makes the updates easier.

Change History (16)

This ticket was mentioned in PR #523 on WordPress/wordpress-develop by rene-hermenau.


3 months ago

In the DocBlocks, we document return values with mixed only.

That is no good practice as it does not show the real available and possible return types.
A better approach is to declare and document explicitly return values separated by a pipe character like this @return bool|string

That is much clearer because developers do not need to check the return values by reading the entire docBlock, reading the whole code of a method, or reading the (lengthy) function description under developer.wordpress.org.

A simplemixed is outdated and makes it hard for every severe developer to start with coding for WordPress as he has to continually check the expected return values when there is a mixed.

As a bonus, the documentation under developer.wordpress.org will be updated as well by this change automatically. It will help developers getting know the correct return values immediately from the docs without reading all the extra special explanations that we added to our docs whenever we use mixed.

That is a huge time saver and makes it much faster for every developer to write solid and better code faster.

If this is accepted for core, I will offer to work on this on all other methods as well.

That will be a long-lasting process due to the number of functions, but it's definitely worth the effort. We should make this change to all methods where we usemixed and make it to our daily little take-care moment when we add new methods to WordPress.

Sample Patch as an example for all methods that use mixed:

get_option() is a special case, though as it automatically unserializes. So it supports all non-scalar types. Most other methods in WordPress uses mixed with only two return types like bool|string representation, which makes the updates easier.

Trac ticket: https://core.trac.wordpress.org/ticket/51278#ticket

#2 @johnbillion
3 months ago

  • Focuses accessibility removed
  • Severity changed from major to normal
  • Version trunk deleted

Thanks for the ticket and the PR @ReneHermi .

There's been quite a bit of work done to remove mixed types in docs over recent releases. See #50768 and its predecessors, and related tickets such as #41756.

I agree that continuing this work makes sense when listing the types really helps the developer. I don't think that @return bool|string|array|object for the get_option() function specifically is much more useful than @return mixed. In addition, the filter on its return value and the recent addition of a default value in #43941 means this function can return a value of any type, including integer, float, etc.

That said, there's over 100 other instances and I'm sure many of those would benefit from more accurate types! So yes, let's replace mixed with proper types when it makes sense to do so.

#3 @ReneHermi
3 months ago

Agree, it's counterproductive to list all available PHP types. I've updated the PR to make this more clear that any data type can be returned.

#4 @johannadevos
2 weeks ago

Having read this discussion and having inspected the accompanying PR, I would say it is ready to be merged:

  • The feedback from @johnbillion has been taken into account.
  • An alternative description has been provided.

#5 @johnbillion
13 days ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to johnbillion
  • Status changed from new to reviewing

#6 @johnbillion
13 days ago

In 49600:

Docs: For clarity, add some information about the return types of get_option().

Props ReneHermi, johannadevos

See #51278

#7 @azaozz
12 days ago

[49600] is a good addition but there are some "gotchas" there which are worth documenting more, imho. Most importantly that options are always saved as strings in the DB. Example:

add_option( 'my_test_option__false', false );
add_option( 'my_test_option__true', true );
add_option( 'my_test_option__0', 0 );
add_option( 'my_test_option__1', 1 );
add_option( 'my_test_option__str_0', '0' );
add_option( 'my_test_option__str_1', '1' );

Then doing var_dump( get_option( 'my_test_option...' ) ); will output:

string(0) ""
string(1) "1"
string(1) "0"
string(1) "1"
string(1) "0"
string(1) "1" 

This is especially important (big back-compat "gotcha") for PHP 8.0 as string to number comparison was changed there, see: [48960].

There's already: Any scalar values will be returned as strings. but having some examples would be better :)

Last edited 7 days ago by SergeyBiryukov (previous) (diff)

#8 @johnbillion
12 days ago

  • Owner johnbillion deleted

Agreed, this can be improved further. Worth noting that get_option() does return correctly typed values for a value that doesn't originate from the database storage, for example the value from the $default parameter when the option doesn't exist, or the return value from any of its filters (pre_option_{$option}, default_option_{$option}, and option_{$option}).

#9 @johnbillion
12 days ago

  • Keywords needs-patch added; has-patch removed

This ticket was mentioned in PR #746 on WordPress/wordpress-develop by rene-hermenau.


11 days ago

  • Keywords has-patch added; needs-patch removed

UPDATED: Elaborate documentation of get_option() due to the possible exceptional cases by using filters and $default

In the DocBlocks, we document return values with mixed only.

That is no good practice as it does not show the real available and possible return types.
A better approach is to declare and document explicitly return values separated by a pipe character like this @return bool|string

That is much clearer because developers do not need to check the return values by reading the entire docBlock, reading the whole code of a method, or reading the (lengthy) function description under developer.wordpress.org.

A simple mixed is outdated and makes it hard for every severe developer to start with coding for WordPress as he has to continually check the expected return values when there is a mixed.

As a bonus, the documentation under developer.wordpress.org will be updated as well by this change automatically. It will help developers getting know the correct return values immediately from the docs without reading all the extra special explanations that we added to our docs whenever we use mixed.

That is a huge time saver and makes it much faster for every developer to write solid and better code faster.

If this is accepted for core, I will offer to work on this on all other methods as well.

That will be a long-lasting process due to the number of functions, but it's definitely worth the effort. We should make this change to all methods where we use mixed and make it to our daily little take-care moment when we add new methods to WordPress.

Sample Patch as an example for all methods that use mixed:

get_option() is a special case, though as it automatically unserializes. So it supports all non-scalar types. Most other methods in WordPress uses mixed with only two return types like bool|string representation, which makes the updates easier.

Trac ticket: https://core.trac.wordpress.org/ticket/51278

This ticket was mentioned in PR #747 on WordPress/wordpress-develop by rene-hermenau.


11 days ago

Update docBlock of get_option() to better explain the expected return values, taking into account all the exceptions like using $default or the internal filters

Trac ticket: https://core.trac.wordpress.org/ticket/51278

#12 @WP-Staging
11 days ago

I've created another patch and improved docBlock further by applying your suggestions.
I tried to simplify the description and the wording to describe and added another example of showing the condition when a return value can be a scalar one.


Last edited 11 days ago by WP-Staging (previous) (diff)

#13 @prbot
10 days ago

azaozz commented on PR #747:

Added some suggestions/changes. Also may be good to mention that as non-scalar option values are serialized before saving, they remain unchanged. For example adding array( true, false, 0, 1 ) will return the exact same array.

#14 @prbot
10 days ago

rene-hermenau commented on PR #747:

Added some suggestions/changes. Also may be good to mention that as non-scalar option values are serialized before saving, they remain unchanged. For example adding array( true, false, 0, 1 ) will return the same array.

@azaozz Excellent points! I feel ashamed for overlooking that strings are scalar. You've passed my test:-)

get_option() is a very tricky method with all these possible different types of returns. We are right on time to work out this improvement. I've added your suggestions. Please have a look at them.

#15 @johnbillion
8 days ago

  • Milestone changed from Future Release to 5.7
Note: See TracTickets for help on using tickets.