Unnecessay If else block in core/lib/Drupal/Core/Archiver/ArchiveTar.php

CommentFileSizeAuthor
#11 Unnecessay_If_else_block-2818249-11.patch719 bytesPatil_kunal27
#6 Unnecessay_If_else_block-2818249-6.patch730 byteslomasr
#4 Unnecessay_If_else_block-2818249-3.patch722 byteslomasr
#2 Unnecessay_If_else_block-2818249-2.patch845 bytesPatil_kunal27

Comments

patil_kunal27 created an issue. See original summary.

Patil_kunal27’s picture

Patil_kunal27 commented
StatusFileSize
new Unnecessay_If_else_block-2818249-2.patch845 bytes

Removed Unnecessary If else block

sam152’s picture

sam152 commented
Status: Active » Needs work

There are some whitespace issues in the patch.

lomasr’s picture

lomasr commented
StatusFileSize
new Unnecessay_If_else_block-2818249-3.patch722 bytes

Patch worked cleanly but some white space issues. Removed in the patch.

sam152’s picture

sam152 commented 
+++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
@@ -2320,11 +2320,9 @@ public function _openAppend()
+ if (fread($this->_file, 512) == ARCHIVE_TAR_END_BLOCK) {
+ fseek($this->_file, $v_size - 512);
+ }

This is still not indented properly.

lomasr’s picture

lomasr commented
StatusFileSize
new Unnecessay_If_else_block-2818249-6.patch730 bytes

Sorry about that.

sam152’s picture

sam152 commented
Status: Needs work » Needs review

The last submitted patch, 4: Unnecessay_If_else_block-2818249-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: Unnecessay_If_else_block-2818249-6.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) commented

You sure that this "Unnecessay If else" problem? Look on prev comment:

// We might have zero, one or two end blocks.
// The standard is two, but we should try to handle
// other cases.

highlight:

if (fread($this->_file, 512) == ARCHIVE_TAR_END_BLOCK) { # TWO BLOCKS
    fseek($this->_file, $v_size - 1024);
} elseif (fread($this->_file, 512) == ARCHIVE_TAR_END_BLOCK) { # ONE BLOCK
    fseek($this->_file, $v_size - 512);
}
# ZERO BLOCK if false with both check

You can looks on this like:

fseek($this->_file, $v_size - 1024);
$first_block = fread($this->_file, 512); // happens an implicit fseek via fread
$second_block = fread($this->_file, 512);

if($first_block == ARCHIVE_TAR_END_BLOCK){
    fseek($this->_file, $v_size - 1024);
}
elseif($second_block == ARCHIVE_TAR_END_BLOCK){
    fseek($this->_file, $v_size - 512);
}
else {
    fseek($this->_file, $v_size - 0);
}

It seems proposed patch breaks this logic. But the code of this file is definitely in need of refactoring.

Patil_kunal27’s picture

Patil_kunal27 commented
StatusFileSize
new Unnecessay_If_else_block-2818249-11.patch719 bytes

Removed the white spaces problem from previous patch

Anonymous’s picture

Anonymous (not verified) commented

It looks like Support request. If my explanation by #10 is not clear for you, сan you explain the algorithm after this patch, please?

Anonymous’s picture

Anonymous (not verified) commented
Category: Bug report » Support request
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: Unnecessay_If_else_block-2818249-11.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Fixed

It seems none of the participants need support or discussion :). Change to Fixed, because code has not "unnecessay if else block", see #10. Please re-open, if you think otherwise. Thanks!

Status: Fixed » Closed (fixed)

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