Currently the code looks like this
/**
* Performs a backup.
*
* @param \Drupal\Core\FileTransfer\FileTransfer $filetransfer
* Object which is a child of FileTransfer.
* @param string $from
* The file path to copy from.
* @param string $to
* The file path to copy to.
*
* @todo Not implemented.
*/
public function makeBackup(FileTransfer $filetransfer, $from, $to) {
}
Added in #538660: Move update manager upgrade process into new authorize.php file (and make it actually work).
Remove the method and associated code
or
No tests are breaking, yet this method is called, meaning, we need to write tests and also actually implement this.
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 2474355-nr-bot.txt | 131 bytes | needs-review-queue-bot |
| #4 | 2474355-4.patch | 1.94 KB | googletorp |
Comments
Comment #1
googletorp commentedComment #2
xjmThanks @googletorp for filing this! Retitling to what I think the situation actually is. :)
So the fact that this method is unused means we should probably do a bit of research -- git blame the line that added it, search the issue queue for the method name.
We also need to decide what to do with this broken hunk of code:
The
make_backuparg is clearly not tested anywhere since this code path would have broken because of #2121863: There is no FileTransferInterface. So we should either fix it or remove it. But let's check how and why it was added to decide which. Thanks!Comment #3
googletorp commentedI've tried to read through the code and play around the UI.
I guess the idea was that when updating or installing a new module through the UI, that it should be possible to create a backup (database backup?). If this was possible, then there also should be an interface to use the backup. For what I can tell, none of this
For what I can tell, it's not possible to set the make_backup arg in any way, so I guess we just should be able to delete the code, which is what makes the most sense for me.
I've made a patch for it, let me know what you think.
Comment #4
googletorp commentedPatch from #3 was incomplete, you can just disregard that completely.
Comment #7
mgiffordNeeds re-roll.
Comment #19
quietone commentedThis still needs to be done.
The function makeBackup was added in Oct 2009 in #538660: Move update manager upgrade process into new authorize.php file (and make it actually work). Reading that issue 'backup' is only mention in the first comment and again in 126. It has been sitting at needs implementation since then. In #3 @googletorp recommends removal.
Updated the IS to emphasize a decision on removal or implementation is needed.
Comment #21
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #23
dwwNow that #3491731: [META] Remove the ability to update modules and themes via authorize.php and child issues are done, all this code is deprecated and will be removed in D12.
Comment #24
xjmSaving credits according to our core issue credit guidelines. Thanks!