WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 7 weeks ago

#53284 assigned defect (bug)

`error()` method passed too many parameters

Reported by: desrosj Owned by: sainthkh
Milestone: 5.9 Priority: normal
Severity: normal Version: 3.7
Component: Upgrade/Install Keywords: good-first-bug has-patch needs-refresh
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].

Change History (7)

#1 @desrosj
2 months ago

  • Version set to 3.7

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


2 months ago

  • Keywords has-patch added; needs-patch removed

#3 @sainthkh
2 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 2 months ago by sainthkh (previous) (diff)

#4 @desrosj
2 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
2 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
2 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
8 weeks 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 7 weeks ago by desrosj (previous) (diff)
Note: See TracTickets for help on using tickets.