Make WordPress Core

Opened 3 years ago

Closed 2 years 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 2 years ago.
Patch created for error() method passed too many parameters.
53284.2.diff (825 bytes) - added by devutpol 2 years 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
3 years ago

  • Version set to 3.7

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


3 years ago
#2

  • Keywords has-patch added; needs-patch removed

#3 @sainthkh
3 years 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 comment said 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?

Version 0, edited 3 years ago by sainthkh (next)

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

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

  • Owner sainthkh deleted

No problem at all, thanks for letting us know.

#10 @hellofromTonya
2 years 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
2 years ago

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

#11 @devutpol
2 years ago

  • Keywords has-patch added; needs-patch removed

@devutpol
2 years 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
2 years 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
2 years 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.