Make WordPress Core

Opened 7 years ago

Closed 3 years ago

#40006 closed enhancement (fixed)

$response var in comments not an array but an object

Reported by: davidmosterd's profile davidmosterd Owned by: audrasjb's profile audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version: 3.8
Component: Upgrade/Install Keywords: has-patch
Focuses: docs Cc:

Description

In the file wp-admin/includes/update.php there is an action do_action( "in_plugin_update_message-{$file}", $plugin_data, $response ); . The comments declares $response as an array but its and object containing the plugin data. The list of plugin data also seems somewhat incomplete. But I'm unsure if all fields I see are always present. The property "tested" seems useful to add at least.

Attachments (3)

40006.diff (1.0 KB) - added by davidmosterd 7 years ago.
A Git to SVN patch that I hoped that worked :)
40006.1.diff (721 bytes) - added by audrasjb 3 years ago.
Upgrade/Install: Docs: Correct the documentation of the $response parameter in the in_plugin_update_message-{$file} action.
40006.2.3.diff (1.4 KB) - added by audrasjb 3 years ago.
Docs: Add missing parameters available in the in_plugin_update_message-{$file} filter. Follow-up to [51733].

Download all attachments as: .zip

Change History (23)

@davidmosterd
7 years ago

A Git to SVN patch that I hoped that worked :)

#1 @davidmosterd
7 years ago

  • Keywords has-patch added

#2 @johnbillion
7 years ago

  • Component changed from Comments to Upgrade/Install
  • Keywords needs-testing added
  • Version trunk deleted

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


3 years ago

#4 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing
  • Version set to 3.8

Hi there, welcome to WordPress Trac!

Thanks for the ticket, sorry it took so long for someone to get back to you.

Setting the version to 3.8, as the documentation was added in [26540] / #26252.

The in_plugin_update_message-{$file} action itself was added in [11193] / #9553.

A corresponding in_theme_update_message-{$theme_key} action for themes was added in [16141] / #14897.

It looks like the $response parameter here is an object for plugins, but is indeed an array for themes. I think it's worth discussing whether we want to make it consistent for both, i.e. make it an array for plugins too, to match the documented value.

On second thought, since the action was added more than 12 years ago, changing the parameter type now would break any custom code relying on it being an object, so perhaps we should just accept the inconsistency and correct the documentation as suggested in 40006.diff. A brief search in the Plugin Directory shows that the action is used in quite a few plugins.

#5 follow-up: @audrasjb
3 years ago

  • Keywords needs-refresh added; needs-testing removed

Indeed, the first step is to update the DocBlock to reflect the actual parameter type.

Then it would also be useful to add the missing parameters available in the $response object. But I think it's better to separate those two steps.

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

@audrasjb
3 years ago

Upgrade/Install: Docs: Correct the documentation of the $response parameter in the in_plugin_update_message-{$file} action.

#6 @audrasjb
3 years ago

  • Keywords needs-refresh removed

#7 @SergeyBiryukov
3 years ago

In 51733:

Docs: Correct documentation for the in_plugin_update_message-{$file} filter.

The $response parameter is an object, not an array.

This is a minor inconsistency with the corresponding in_theme_update_message-{$theme_key} action for themes, where the $response parameter is an array.

For backward compatibility, it is safer not to change the parameter type at this point, but to make sure the documentation is correct.

Follow-up to [11193], [16141], [26540].

Props davidmosterd, audrasjb, SergeyBiryukov.
See #40006.

#8 in reply to: ↑ 5 @SergeyBiryukov
3 years ago

Replying to audrasjb:

Then it would also be useful to add the missing parameters available in the $response object.

Keeping the ticket open for this :)

@audrasjb
3 years ago

Docs: Add missing parameters available in the in_plugin_update_message-{$file} filter. Follow-up to [51733].

#9 @audrasjb
3 years ago

Just added the missing parameters :)

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


3 years ago

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


3 years ago

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


3 years ago

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


3 years ago

#14 follow-up: @costdev
3 years ago

@type string $requires Required WordPress version.
@type string $tested Higher WordPress version the plugin has been tested with.

For consistency with another docblock in core (see src/wp-admin/includes/plugin.php:33), I suggest:

@type string $requires Specify the minimum required WordPress version.
@type string $tested Specify the latest version of WordPress that the plugin has been tested with.

#16 @audrasjb
3 years ago

  • Owner changed from SergeyBiryukov to audrasjb

Thanks @costdev, I added them in the new PR. Self-assigning for commit.

#17 @audrasjb
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 52212:

Docs: Add missing parameters in in_plugin_update_message-{$file} filter.

Follow-up to [51733].

Props costdev, audrasjb, SergeyBiryukov.
Fixes #40006.

#19 in reply to: ↑ 14 @SergeyBiryukov
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It looks like [52212] accidentally reverts [51733], will commit a fix shortly.

Replying to costdev:

@type string $requires Required WordPress version.
@type string $tested Higher WordPress version the plugin has been tested with.

For consistency with another docblock in core (see src/wp-admin/includes/plugin.php:33), I suggest:

@type string $requires Specify the minimum required WordPress version.
@type string $tested Specify the latest version of WordPress that the plugin has been tested with.

Thanks! Consistency is great, but that DocBlock is an example of plugin headers, which is why it says "specify". Since we're dealing with a do_action() call here, this is not the place to specify these values, we can only work with whatever values are already passed in the $response object. So I think in this case it would be better to follow the DocBlock for the return result of get_plugin_data(), or the DocBlock for the update_plugins_{$hostname} filter, which documents pretty much the same values.

#20 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 52224:

Docs: Restore [51733], accidentally reverted in [52212].

Document some more $response values in the in_plugin_update_message-{$file} filter:

  • banners_rtl
  • requires_php

Follow-up to [51733], [52212].

Fixes #40006.

Note: See TracTickets for help on using tickets.