Remove unused variable $result from MoveTest

Created on 5 January 2023, almost 2 years ago
Updated 1 February 2023, almost 2 years ago

Problem/Motivation

Remove unused variable $result from MoveTest. There are couple of instances

    try {
      $result = $this->fileRepository->move(clone $source, $source->getFileUri(), FileSystemInterface::EXISTS_ERROR);
      $this->fail('expected FileExistsException');
    }
    try {
      $result = $this->fileRepository->move(clone $source, $target->getFileUri(), FileSystemInterface::EXISTS_ERROR);
      $this->fail('expected FileExistsException');
    }

Steps to reproduce

Proposed resolution

Remaining tasks

Investigate the history of the variable. Why was it added and what happened that made it unused.

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Fixed

Version

10.1

Component
File system 

Last updated 2 days ago

Created by

🇮🇳India anmolgoyal74

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇮🇳India anup.sinha Bengaluru

    Hi @quietone,

    Old code in 9.2.x branch:
    // Copy the file over itself. Clone the object so we don't have to worry
    // about the function changing our reference copy.
    $result = file_move(clone $source, $source->uri, FILE_EXISTS_REPLACE);
    $this->assertFalse($result, t('File move failed.'));
    $this->assertEqual($contents, file_get_contents($source->uri), t('Contents of file were not altered.'));

    The file was changed in 9.3.x release in this commit - https://git.drupalcode.org/project/drupal/-/commit/64074071d1c78274df253...
    Issue link - https://www.drupal.org/project/drupal/issues/3223209

    Here deprecated file_move() function was replaced with service method fileRepository->move(). Now earlier there was an assertFalse() as file_move() function returns TRUE/FALSE but the new service move() function either returns the File entity or throws exception in case of errors. So we cannot use the old assertFalse() check anymore.

    So as per my analysis current patch is fine as no test coverage is removed. Request you to check the mentioned commit and let me know please if there is any observation.

    Thanks & Regards,
    Anup

  • Status changed to Needs work almost 2 years ago
  • The 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.

  • 🇮🇳India anup.sinha Bengaluru

    Hi @Prabu,

    The automated test bot actually tried to test your merge request instead of the patch. As a a result it failed as there are no changes in that merge request. So request you to close that merge request if you are not planning to use it and put the issue status to needs review.

    Thanks & Regards,
    Anup

  • Status changed to Needs review almost 2 years ago
  • Status changed to RTBC almost 2 years ago
  • 🇬🇧United Kingdom longwave UK

    Thanks for the analysis in #9. We no longer need the variable now we have an exception, so this is change is correct and ready to commit.

    • catch committed bf30a58e on 10.1.x
      Issue #3331212 by PrabuEla, anup.sinha, anmolgoyal74, longwave, quietone...
  • Status changed to Fixed almost 2 years ago
  • 🇬🇧United Kingdom catch

    Committed/pushed to 10.1.x, thanks!

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024