Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#51278 reviewing enhancement

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

Reported by: renehermi's profile ReneHermi Owned by:
Milestone: Future Release Priority: normal
Severity: major Version:
Component: General Keywords: has-patch dev-feedback
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.

Attachments (1)

51278.diff (4.0 KB) - added by azaozz 3 years ago.

Download all attachments as: .zip

Change History (29)

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


4 years ago
#1

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
4 years 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
4 years 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
3 years 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
3 years ago

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

#6 @johnbillion
3 years ago

In 49600:

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

Props ReneHermi, johannadevos

See #51278

#7 @azaozz
3 years 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 3 years ago by SergeyBiryukov (previous) (diff)

#8 @johnbillion
3 years 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
3 years ago

  • Keywords needs-patch added; has-patch removed

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


3 years ago
#10

  • 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.


3 years ago
#11

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
3 years 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 3 years ago by WP-Staging (previous) (diff)

azaozz commented on PR #747:


3 years ago
#13

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.

rene-hermenau commented on PR #747:


3 years ago
#14

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
3 years ago

  • Milestone changed from Future Release to 5.7

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by lukecarbis. View the logs.


3 years ago

#19 @lukecarbis
3 years ago

  • Keywords dev-feedback added

#20 @hellofromTonya
3 years ago

  • Milestone changed from 5.7 to 5.8

5.7 Beta 1 is happening shortly. Ran out of time to get this ticket into the release. Punting to 5.8.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#21 @ReneHermi
3 years ago

This is only a change of the docBlock. It does not require any UnitTests or similar as it does not touch any real code. It just needs a decision by a maintainer.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 years ago

#23 follow-up: @desrosj
3 years ago

  • Milestone changed from 5.8 to Future Release

Today is the feature freeze for the 5.8 release. Unfortunately, a committer has not had a chance to review this for final inclusion.

I'm going to punt this to Future Release to prevent having to punt repeatedly. But please do keep up the discussion and refinement. Documentation changes can happen after the feature freeze and before the RC point of the release cycle. if a committer has a chance to review during that time, this can be moved back.

#24 @ReneHermi
3 years ago

  • Severity changed from normal to major

It's an important update to the docs. The lack of getting someone to review this quickly proves the necessity of why this docBlock update is so important. The one who reviews this might think she will have to go through a line-by-line check of get_option() because it does have so many exceptions instead of clear return values. I'd say it's not necessary to do this and this PR can be added into the core without such a deep code inspection.

Very talented people have already looked into this like @johnbillion

Even if we overlooked something in this PR (which I do not believe) it will still improve the documentation tremendously with less effort. At worse it will not cover and document all eventualities and return values but the current docBlock only covers a fraction of them.

So instead of refining and delaying this PR more and more until so much time has passed that it is not going to be anywhere, I highly recommend bringing it into core asap. Then we can still fine-tune it with smaller PR's which are easier to review if it should be necessary.

I've changed severe status from normal to major and hope someone will get on to this soon. We are nearing 10 months for such a "simple" PR. I do not expect someone to look into this if it takes more time.

#25 in reply to: ↑ 23 @azaozz
3 years ago

Replying to desrosj:

Today is the feature freeze for the 5.8 release.

Right, the current patch is an enhancement but only for the documentation, doesn't touch any code. Can be committed at any time even during RC. Thinking it's almost ready, just needs a little clarification in couple of places. Will try to review in the next day or two.

However this ticket is for all @return mixed in core. Last count is about 170 and most don't list the possible types. A Future Release milestone makes sense.

Last edited 3 years ago by azaozz (previous) (diff)

@azaozz
3 years ago

#26 @azaozz
3 years ago

In 51278.diff: tweaked, improved and cleaned up the get_option() docblock a bit. Thinking this is about ready for commit. The full text is:

/**
 * Retrieves an option value based on an option name.
 *
 * If the option does not exist, and a default value is not provided,
 * boolean false is returned. This could be used to check whether you need
 * to initialize an option during installation of a plugin, however that
 * can be done better by using {@see add_option} which will not overwrite
 * existing options.
 *
 * Not initializing an option and using the boolean false as a return value
 * is a bad practice as it triggers an additional database query.
 *
 * The type of the returned value can be different from the type that was passed
 * when saving or updating the option. If the option value was serialized,
 * then it will be unserialized when it is returned. In this case the type will
 * be the same. For example, storing a non-scalar value like an array will
 * return the same array.
 *
 * In most cases non-string scalar and null values will be converted and returned
 * as string equivalents.
 *
 * Exceptions:
 * 1. When the option has not been saved in the database, the default value
 *    {@see get_option} is returned if provided. If not, boolean `false` is returned.
 * 2. When one of the Options API filters is used: {@see pre_option_{$option}},
 *    {@see default_option_{$option}}, and {@see option_{$option}}, the returned
 *    value may not match the expected type.
 * 3. When the option has just been saved in the database, and {@see get_option}
 *    is used right after, non-string scalar and null values are not converted to
 *    string equivalents and the original type is returned.
 *
 * Examples:
 *
 * When adding options like this: `add_option( 'my_option_name', 'value' );`
 * and then retrieving them with `get_option( 'my_option_name' )`, the returned
 * values will be:
 *
 * `false` returns string(0) ""
 * `true`  returns string(1) "1"
 * `0`     returns string(1) "0"
 * `1`     returns string(1) "1"
 * `'0'`   returns string(1) "0"
 * `'1'`   returns string(1) "1"
 * `null`  returns string(0) ""
 *
 * When adding options with non-scalar values like
 * `add_option( 'my_array', array( false, 'str', null ) );`, the returned value
 * will be identical to the original as it is serialized before saving
 * it in the database:
 *
 * array(3) {
 *    ["false"] => bool(false)
 *    ["str"] => string(3) "str"
 *    ["null"] => NULL
 * }
 *
 * @since 1.5.0
 *
 * @global wpdb $wpdb WordPress database abstraction object.
 *
 * @param string $option  Name of the option to retrieve. Expected to not be SQL-escaped.
 * @param mixed  $default Optional. Default value to return if the option does not exist.
 * @return mixed Value of the option. A value of any type may be returned, including
 *               scalar (string, boolean, float, integer), null, array, object.
 *               Scalar and null values will be returned as strings as long as they originate
 *               from a database stored option value. If there is no option in the database,
 *               boolean `false` is returned.
 */

#27 @ReneHermi
3 years ago

@azaozz Very well! This is clear and very comprehensible. Will save thousands of man-hours. Looking forward to see this merged.

#28 @azaozz
3 years ago

In 51050:

Docs: Improve documentation for get_option(). Clean up, clarify the returned types and the exceptions, and add few examples.

Props ReneHermi, johnbillion, azaozz
See #51278

Note: See TracTickets for help on using tickets.