Opened 4 years ago
Closed 3 years ago
#53284 closed defect (bug) (fixed)
`error()` method passed too many parameters
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (15)
This ticket was mentioned in PR #1306 on WordPress/wordpress-develop by sainthkh.
4 years ago
#2
- Keywords has-patch added; needs-patch removed
#3
@
4 years ago
I thought about there are 3 possible fixes for this ticket.
- Call
error
twice.- Downside: It's usually not called twice.
- Create a new
WP_Error
object with the first message,Installation failed
.- Downside: It's too complicated.
- 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?
#4
@
4 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 fromWP_Upgrader->fs_connect()
. - A
WP_Error
with thelocked
error code if another update is already in progress. - A
WP_Error
with theno_package
ordownload_failed
code fromWP_Upgrader->download_package()
. - A
WP_Error
with theincompatible_archive
code fromWP_Upgrader->unpack_package()
. - A
WP_Error
with thecopy_failed_for_update_core_file
orcopy_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
@
4 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
@
4 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
@
4 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!
#8
@
4 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.
#10
@
3 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.
@
3 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.
Trac ticket: https://core.trac.wordpress.org/ticket/53284