Make WordPress Core

Opened 17 months ago

Closed 9 months ago

#53284 closed defect (bug) (fixed)

`error()` method passed too many parameters

Reported by: desrosj's profile desrosj Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.0 Priority: normal
Severity: normal Version: 3.7
Component: Upgrade/Install Keywords: good-first-bug has-patch
Focuses: Cc:

Description

In src/wp-admin/includes/class-wp-automatic-updater.php, there is a call to the `$skin->error()` method passing two parameters, but this method only accepts 1.

Introduced in [25421].

Attachments (2)

53284.diff (742 bytes) - added by devutpol 11 months ago.
Patch created for error() method passed too many parameters.
53284.2.diff (825 bytes) - added by devutpol 11 months ago.
I thought of a different solution and uploaded 53284-2.diff. WP_Error object is now stored in $upgrade_result with a new error and I believe it now works correctly. This is my first submission for WordPress Core so I apologize if I have made any mistakes with the Trac workflow.

Download all attachments as: .zip

Change History (15)

#1 @desrosj
17 months ago

  • Version set to 3.7

This ticket was mentioned in PR #1306 on WordPress/wordpress-develop by sainthkh.


17 months ago

  • Keywords has-patch added; needs-patch removed

#3 @sainthkh
17 months ago

I thought about there are 3 possible fixes for this ticket.

  1. Call error twice.
    • Downside: It's usually not called twice.
  1. Create a new WP_Error object with the first message, Installation failed.
    • Downside: It's too complicated.
  1. Make WP_Upgrader_Skin->error handle multiple arguments.
    • Downside: Most error functions in WP only handles one argument. It's not consistent.

I chose 1. Because it's simple.

The github-bot told me to add an automated test. But I'm a newbie here and checked the tests folder and it seems that the tests don't exist for this class. If I missed something, can you help me write tests for this?

Last edited 17 months ago by sainthkh (previous) (diff)

#4 @desrosj
17 months ago

  • Milestone changed from Future Release to 5.8
  • Owner set to sainthkh
  • Status changed from new to assigned

Thanks for working on a patch, @sainthkh!

The upgrade code unfortunately is extremely lacking in tests. In this case, this change could be made without adding unit tests, but we should just examine it extra carefully to be sure no issues will result.

With that in mind, I took a look into what $upgrade_result can actually be set to so we could document it.

It looks like these are the scenarios:

  • The new WordPress version when everything works.
  • false
  • One of several WP_Error objects from an inability to read or manipulate the filesystem from WP_Upgrader->fs_connect().
  • A WP_Error with the locked error code if another update is already in progress.
  • A WP_Error with the no_package or download_failed code from WP_Upgrader->download_package().
  • A WP_Error with the incompatible_archive code from WP_Upgrader->unpack_package().
  • A WP_Error with the copy_failed_for_update_core_file or copy_failed_space code if the new version fails to copy to the correct locations.

I think having knowledge of the error codes listed above and their messages could be valuable, so let's see about the best way to get both errors logged as feedback in the WP_Upgrader_Skin.

WP_Error objects are able to handle multiple error codes. Calling WP_Error->add() should allow a new error to be added to the object after the one currently stored in $upgrade_result. WP_Upgrader_Skin->error() is also able to handle `WP_Error` objects that contain multiple errors.

That would allow both error messages to be logged while still only calling error() once.

I'm assigning this ticket to you as that's the way to mark a good-first-bug ticket as claimed. I'm also moving it to the 5.8 milestone. Since someone picked up the ticket, it's reasonable to expect that this one can be wrapped up this release!

#5 @desrosj
16 months ago

  • Keywords needs-refresh added

Hi @sainthkh, just checking in to see if you had a chance to take another look at this. The deadline to include this fix in the upcoming 5.8 release is 8 June. No worries if you will not have the time to look before then!

#6 @sainthkh
16 months ago

I'm sorry for the late reply.

I might need more time and I guess I cannot meet the deadline because:

  • the code needs a little bit of refactoring to inject mocks for the test.
  • I'm learning how to use phpunit. And it seems that WP phpunit doesn't support some new features.
  • I have some other things to do.

I'll come back to tackle it next week.

#7 @desrosj
16 months ago

  • Milestone changed from 5.8 to 5.9

@sainthkh No worries at all!

To clarify, I don't think this change requires any unit tests at this time. Today is the beta 1 deadline for 5.8, so I'll move this to 5.9. Feel free to tackle when you have a chance!

Last edited 16 months ago by desrosj (previous) (diff)

#8 @sainthkh
14 months ago

I'm sorry. It's hard for me to alloc time for this ticket.

It seems that it's better to set this ticket to new for others to try.

#9 @johnbillion
14 months ago

  • Owner sainthkh deleted

No problem at all, thanks for letting us know.

#10 @hellofromTonya
11 months ago

  • Keywords needs-patch added; has-patch needs-refresh removed
  • Milestone changed from 5.9 to 6.0

With 5.9 Beta 1 in 2 days and no activity, moving this to 6.0. If a patch is ready before Beta 1, please move it back into the 5.9 milestone for commit consideration.

@devutpol
11 months ago

Patch created for error() method passed too many parameters.

#11 @devutpol
11 months ago

  • Keywords has-patch added; needs-patch removed

@devutpol
11 months ago

I thought of a different solution and uploaded 53284-2.diff. WP_Error object is now stored in $upgrade_result with a new error and I believe it now works correctly. This is my first submission for WordPress Core so I apologize if I have made any mistakes with the Trac workflow.

#12 @SergeyBiryukov
11 months ago

Thanks for the patch!

53284.2.diff looks good to me at a glance and appears to accomplish what comment:4 suggested.

#13 @davidbaumwald
9 months ago

  • Owner set to davidbaumwald
  • Resolution set to fixed
  • Status changed from assigned to closed

In 52539:

Upgrade/Install: Fix parameter count in error call when an automatic core upgrade fails.

During automatic core upgrades, if installation results in a WP_Error, the error method is called on the skin with the details. However, in this case, two parameters are passed to $skin->error, but only one is accepted. This change passes only the running WP_Error instance as the sole parameter to $skin->error.

Also, this change adds an additional error to $upgrade_result before finally being passed to $skin->error, indicating that the installation failed. This adds additional context to the failure.

Props desrosj, sainthkh, devutpol, SergeyBiryukov.
Fixes #53284.

Note: See TracTickets for help on using tickets.