From 766252ccb0a212b6d9a26c802a90c22ba025829b Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Sat, 30 Apr 2022 19:13:17 -0700 Subject: [PATCH] Real Errors Identified in Calculation by Scrutinizer (#2774) * Real Errors Identified in Calculation by Scrutinizer Before Scrutinizer broke, I took a look at the remaining 43 errors which it categorized as 'major'. Most of these were false positives, but, in the case of Calculation and Reader/Xlsx/Chart, I was able to determine that its analysis of some of the problems was correct. There is little point addressing the false positives until it starts working again, but we should fix the real errors. This PR addresses the real errors in Calculation. - A test for `$pCellParent` should have been a test for `$pCellWorksheet`. - A test for `$operand1Data['reference']` should have been a test for `$operand1Data['value']`. - A test for `$operand2Data['reference']` should have been a test for `$operand2Data['value']`. * Fix Attempt to Erroneously Call trim on Array Fix #2780. Warning message is being issued when getting calculated value of cell with value `=INDIRECT(ADDRESS(ROW(),COLUMN()-2))/$C$4`. This appears to be the case for all recent (and probably not so recent) releases; it is not a result of changes to the code base. Fix added to this PR because the erring section of code was proximate to code already changed in the PR. Test added. * Minor Code Changes Apply some suggestions from @MarkBaker --- phpstan-baseline.neon | 30 ---------------- .../Calculation/Calculation.php | 34 ++++++++++-------- .../Reader/Xlsx/Issue2778Test.php | 26 ++++++++++++++ tests/data/Reader/XLSX/issue.2778.xlsx | Bin 0 -> 8646 bytes 4 files changed, 45 insertions(+), 45 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2778Test.php create mode 100644 tests/data/Reader/XLSX/issue.2778.xlsx diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index fc010b55..13ce6f9e 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -20,16 +20,6 @@ parameters: count: 6 path: src/PhpSpreadsheet/Calculation/Calculation.php - - - message: "#^Cannot call method getColumn\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Cell\\|null\\.$#" - count: 2 - path: src/PhpSpreadsheet/Calculation/Calculation.php - - - - message: "#^Cannot call method getCoordinate\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Cell\\|null\\.$#" - count: 2 - path: src/PhpSpreadsheet/Calculation/Calculation.php - - message: "#^Cannot call method getHighestColumn\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\|null\\.$#" count: 1 @@ -40,21 +30,6 @@ parameters: count: 1 path: src/PhpSpreadsheet/Calculation/Calculation.php - - - message: "#^Cannot call method getRow\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Cell\\|null\\.$#" - count: 2 - path: src/PhpSpreadsheet/Calculation/Calculation.php - - - - message: "#^Cannot call method getTitle\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\|null\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/Calculation.php - - - - message: "#^Cannot call method has\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Collection\\\\Cells\\|null\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/Calculation.php - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Calculation\\\\Calculation\\:\\:_translateFormulaToEnglish\\(\\) has no return type specified\\.$#" count: 1 @@ -140,11 +115,6 @@ parameters: count: 1 path: src/PhpSpreadsheet/Calculation/Calculation.php - - - message: "#^Parameter \\#1 \\$parent of method PhpOffice\\\\PhpSpreadsheet\\\\Cell\\\\Cell\\:\\:attach\\(\\) expects PhpOffice\\\\PhpSpreadsheet\\\\Collection\\\\Cells, PhpOffice\\\\PhpSpreadsheet\\\\Collection\\\\Cells\\|null given\\.$#" - count: 1 - path: src/PhpSpreadsheet/Calculation/Calculation.php - - message: "#^Parameter \\#1 \\$str of function preg_quote expects string, int\\|string given\\.$#" count: 1 diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 5f2af864..7a48a7cb 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -3223,10 +3223,11 @@ class Calculation // So instead we skip replacing in any quoted strings by only replacing in every other array element // after we've exploded the formula $temp = explode(self::FORMULA_STRING_QUOTE, $formula); - $i = false; + $notWithinQuotes = false; foreach ($temp as &$value) { // Only adjust in alternating array entries - if ($i = !$i) { + $notWithinQuotes = !$notWithinQuotes; + if ($notWithinQuotes === true) { $value = self::translateFormulaBlock($from, $to, $value, $inFunctionBracesLevel, $inMatrixBracesLevel, $fromSeparator, $toSeparator); } } @@ -3898,10 +3899,11 @@ class Calculation $temp = explode(self::FORMULA_STRING_QUOTE, $formula); // Open and Closed counts used for trapping mismatched braces in the formula $openCount = $closeCount = 0; - $i = false; + $notWithinQuotes = false; foreach ($temp as &$value) { // Only count/replace in alternating array entries - if ($i = !$i) { + $notWithinQuotes = !$notWithinQuotes; + if ($notWithinQuotes === true) { $openCount += substr_count($value, self::FORMULA_OPEN_MATRIX_BRACE); $closeCount += substr_count($value, self::FORMULA_CLOSE_MATRIX_BRACE); $value = str_replace($matrixReplaceFrom, $matrixReplaceTo, $value); @@ -4593,7 +4595,7 @@ class Calculation if (strpos($operand1Data['reference'], '!') !== false) { [$sheet1, $operand1Data['reference']] = Worksheet::extractSheetTitle($operand1Data['reference'], true); } else { - $sheet1 = ($pCellParent !== null) ? $pCellWorksheet->getTitle() : ''; + $sheet1 = ($pCellWorksheet !== null) ? $pCellWorksheet->getTitle() : ''; } [$sheet2, $operand2Data['reference']] = Worksheet::extractSheetTitle($operand2Data['reference'], true); @@ -4602,21 +4604,23 @@ class Calculation } if (trim($sheet1, "'") === trim($sheet2, "'")) { - if ($operand1Data['reference'] === null) { - if ((trim($operand1Data['value']) != '') && (is_numeric($operand1Data['value']))) { + if ($operand1Data['reference'] === null && $cell !== null) { + if (is_array($operand1Data['value'])) { + $operand1Data['reference'] = $cell->getCoordinate(); + } elseif ((trim($operand1Data['value']) != '') && (is_numeric($operand1Data['value']))) { $operand1Data['reference'] = $cell->getColumn() . $operand1Data['value']; - // @phpstan-ignore-next-line - } elseif (trim($operand1Data['reference']) == '') { + } elseif (trim($operand1Data['value']) == '') { $operand1Data['reference'] = $cell->getCoordinate(); } else { $operand1Data['reference'] = $operand1Data['value'] . $cell->getRow(); } } - if ($operand2Data['reference'] === null) { - if ((trim($operand2Data['value']) != '') && (is_numeric($operand2Data['value']))) { + if ($operand2Data['reference'] === null && $cell !== null) { + if (is_array($operand2Data['value'])) { + $operand2Data['reference'] = $cell->getCoordinate(); + } elseif ((trim($operand2Data['value']) != '') && (is_numeric($operand2Data['value']))) { $operand2Data['reference'] = $cell->getColumn() . $operand2Data['value']; - // @phpstan-ignore-next-line - } elseif (trim($operand2Data['reference']) == '') { + } elseif (trim($operand2Data['value']) == '') { $operand2Data['reference'] = $cell->getCoordinate(); } else { $operand2Data['reference'] = $operand2Data['value'] . $cell->getRow(); @@ -4829,7 +4833,7 @@ class Calculation $this->debugLog->writeDebugLog('Evaluation Result for cell %s in worksheet %s is %s', $cellRef, $matches[2], $this->showTypeDetails($cellValue)); } else { $this->debugLog->writeDebugLog('Evaluating Cell %s in current worksheet', $cellRef); - if ($pCellParent->has($cellRef)) { + if ($pCellParent !== null && $pCellParent->has($cellRef)) { $cellValue = $this->extractCellRange($cellRef, $pCellWorksheet, false); $cell->attach($pCellParent); } else { @@ -5281,7 +5285,7 @@ class Calculation } // Named range? - $namedRange = DefinedName::resolveName($range, $worksheet); + $namedRange = DefinedName::resolveName($range, /** @scrutinizer ignore-type */ $worksheet); if ($namedRange === null) { return Information\ExcelError::REF(); } diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2778Test.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2778Test.php new file mode 100644 index 00000000..925b6fa4 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2778Test.php @@ -0,0 +1,26 @@ +load($filename); + $sheet = $spreadsheet->getActiveSheet(); + self::assertSame(6, $sheet->getCell('D1')->getCalculatedValue()); + self::assertSame(63, $sheet->getCell('F1')->getCalculatedValue()); + self::assertSame(10, $sheet->getCell('C10')->getCalculatedValue()); + $spreadsheet->disconnectWorksheets(); + } +} diff --git a/tests/data/Reader/XLSX/issue.2778.xlsx b/tests/data/Reader/XLSX/issue.2778.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..68cbe288410a7133420f01886f3877397d24655b GIT binary patch literal 8646 zcmeHN1zTI&whiu(pv4OWD-@@Ai&oxI~P+s7kxEP2UF))Y#z2Y4-1hIneqXM@caLJ{0GlKRm!LxA`P(jK;asfykW3>l4H3kM?mNasbwVn`S*j$ssLrC8~DhS2Tx4)gLnh zl7lTkvC0zN01pc6+>m3oKsnQGo*9o*%qHDgwGP|OlMxx@=&JO6L?MljCg@+7GCoyk z3f+dQ>|<}S zKP?q_ZZ)NX!XJjNVvLlaJ$N#tWBIk~0>ckiDzR)OHr8ch@PoMzxXm8u*|8{|DuC3} z*c9-V&6`+6LSv%RQsSkv@ul(wVcy~(>vh1S0Emsx!6KHe9UF|yNy`7^#S)zme#K7B zQ%)D(YO#yD=Xdcj{&3H`zefT<{^pZS>Kt^ZaH7e>T@DNGllo4kHqPv9KhOX5#s6Rj z{L70E)}Gi%GZBsQCrRe8p>uEdrA-bTU!^)yRU8UiicO zZU*KTgk!fq(VTzdsfxxY6ryW(uMSJQc63F1#Nd=Fn>x>UA?wNH28(|L zZ7DC1>tCXi`g|l=4;*ECNrpjCLJ>wRk`bugtDv=LbXkKiC9b+x9ai_2FMm6EG|PWF zwR8*TS%iT6?nDOBCubw`*&3fuHZ*5fWa_FG0+w|~xlV$#p86&>?@uJNItgxl9xG;l zQlaI+zvLQy(MO+u=C9qzb2OOa*~5p?4gGZ39~4bpzW9$$GAV=c<`_OBZ{SWs20%yf zuwnm~pSasQSsU5gTmKAQfBFmpJbuBg{AV9kNu#no9B};(IsksR`&>gyo3$?nI)8MA zATDV8p;V?n#{c>}EJ}wY^=xABJzuxa61zlmdu>-Vp8T2-1VTiQj?K`_c8a-}nU;kH z>=A67>kGhS0MATKznp)0nt6h+#h3JnEs#+2`;oFN7crW`a|+Hi8IsTW0)vx-%2~nt z%60?CdZnFIkUWnlU+BZVc^%*|&-b-^nDMkg@aT94-h)f|5OA)KUx0nm(O)KuPG;6mmaTZ^sOhdk=?%onqczI}sW9iz`V&=YLud87mk zsdufj73A&L(eGiK;NEghfu zm#F0wN$5!&o=qrfe)chN^^eY%4NBzF0UdBP3VA zWYS{UrHta}TJ5zUMi%QNh;Ab!ZZsiImDOH5MEh|+FdYD^BCM3KTag>&HBdcmEWx^Y zBW$MAVAxDsO^orXcUB6TW5(Mm#enVUoSye?_{$Il-@%g+Oe!1;8@aw`gZUha)2^h} z5UNiN;;t%88YaZss3mC0mc!;kzIzR?*T4L`-41XGuLGMLfupO)2={=b9)oGE# zXaRTU+Gg6?;d^?9YzRfwDnTdt2fU6tb@AX?)(>}N#s>ihkD|q%9in+IFwS(JE>i~6 zJ8fph-%4Kbm%?CM0x$f-~V^`2& zglcZWihd$VUs|=WrFKpEPqd5WV5$14bBu9#hOTEg0WYG7ky+LTrU|h^nIIgd3FPQB80% zh&$-SgUou^ z>qr%sXG2Oj@g1@h2UiIxgBQ|MtzXMR;y^OGjAESWuqPP_Ah)oI;&vF%Vwpo0IvcqZ z&q7*h{kVi-U$}bYG8Wauq=0zi-UohTH{l-l&>ct*im*jH(b-1oTlUm(jm5|)9N#8C zq_eb(L7^BtYpWp_uR5~H6Cd`?s$CWU(N2qB5%p>CYY`ODBG3@eYT^88d$rx@XgwKI zd*jSLuh&^+;PmeFG{3vg755=#%Iw9L!(cIVJs$Yy72jwjvZk@0U#|r7`V>uUdEy|| zih}6-SIFAFp`AGh%oCY>*o-4uJcfxCrINEl5^$iqNfPp!v`^NxWA^J@004|)>4Y9o zG-+-=$iy*%NkBTXL9P@8Uc{!4k#H{2AXVEw)V~vUEfEfC=9uu3(|aFM%PaE~m%7A+ zVQxR%U~1|id!Fz8@hri{dP5&0UT&`N<<{CtcQV>Dv*wsMA*PINN`0Rnvw8adyEjK` zStRlA>Q-Ch?$5%N{I0H?g@6n?9xGRA@&4Y|msc@tbNoACc){D8*jR1`GRoW39Z3844tp;fnQ{Z@w#GkOe;ho@iPavODtvluI# zexwmnOW%XNHL7zTfLjlvHs zxF|&~vy$MOqTm5b0n?P!=6AMK9rG09F~d4DSJ^g~mP%nL8X_d|WnI zZu>>V$AoqzRCH3@FQQ_lchG_jV#6&U$vH*&E?UxruC-J8g<#DnilkQ*v-7p_P;2#Q z6XAC#$}MI@w8*Zv3n=T8T7pE@@T;EmxZV104)LR*fedhB+-(50cogl1Ai2ZBWOvcqywoPQi60V0o!ZQ270;+RLk!|;i zlMR(F+LJ=e_3yNW%8Gpm_Mb5=i|{Db7<-t1!I}}e^($N2zt`*JgAH!f&V@pP!$_d6 zO>XggYzvGb6TbEN9%;}iq3~lKD$$}jjdIw!6$@YVGS?E*gBHsuHEKrFZ*;VUx9GzL zk^1W7=I2e-v)?8ZHb69 zp9$1$TeZJS%Rzo$im5D|>NtRlk~;pnOJG{s-icv~8$e7jh|NU1kfh6}s5HVR*Yq-? zQA}KpFF7PN3`DjnEOb_sd=lddMXwN3YBRceLdZil%m=b&rY&m>WN1?v>k+VP7h4Gb zTo1}@TEN)7S>ehR+jnT)f9`_WXHJ)B#(uNIG|0MM819iGpc|=JipmM%cV&v4ugSi9 zL~c>YBk2hJA_`p8c6yxf`fV%)@&YzjuKc0&rs1@9aan^7#lRdW=Q&d?Q(L;Z z1AQTj;I`d5(jHqLH^0k5D4MF4!M*F0NV}TcsAZwnq`ZW>$y8(XRu;=K8MH8htcq(( zV;)t{;+1uUYeM;#M~+fczM(<+$}FhdI$5s zG%k1(b*_f;K4VqS^y^%kg2`5Fxo?-m9ZW-gJGHl5tuG4dBA9BfXU)?PvR{lpzdL%~ zRO=fMnfAhtM2eM-`z)}eDguhfF*Wxcj{_HByp?1Uwb~0xo}zs=lD~G=Hmt&e-BzLp zeBu?Xx5=jy{G-OX_`O~km6uqj7G|ZP=5bI|@siDJGQH|AM`Sr>OFLgj(>OU!MXd&S||ug$rK?lIyvr0o_;+A@LaHNSSts0I#GdATO*{jd|nua~0C~ zzdlx>oOSZTd8N4KrTV1elU%;$am)^$T$Hk6c!N3(Sj#N`$HD==*IIKp)^)ZjbBc5$ zmdoPPak=5Qi`8iIs~q3+-=|Q@q{bY=o@HM@J?|}eJVrg@w%sF@$x6a%tk&@0IwY~; zUL~T*F}q70i|Ms1M@op*J5$i$8&Gc0+iUCK01SkVM;|JDW1cio{&=iBg#V`hRCm{p z>zyob2o8336t>R_$vTBuDN+gT!Fl^?y<%7O9&S+nJozb7Osf1AGoL}srj%u3CuN7t zBX6#(u@DO`+>#lZbmU}bnIYY#aFGP5;dL0^7q_pLp^L;SW#Ky({tNqg`bya0hGgLw zg+1C0W&JjHKp2moJi|%&$3jkjKWqM_q7D7pcqN>&SvQIUhPJIRJtNw)5mH`sLHFs?mOTYuUG`i!$p1Bp@b;Jh-A3W1*0dRq3QS%hC4L3 z+R{tnb7#O4E8RwRGR+8myrY#Zfv$%_4k9Gr?Uc`FusUwFrmr5|4nB)t&DyPMSAC!d zstwMeN*9=U%RL)ocK7##e**MnAM=0MB1#=K(5C6c+IRe<`K4$;qo%=IQEJ2ojgqsH z9**>0hK!qX?-=!`*tZf=Y^zs!6tN})Xhssah0V?Q*Dl={`MnBVyHkc7X0SL4-$3Y- zq1gB5>E04CWC|uSElE7x-gC?I{TEBIdW$`DZde$(~D?in$iiZkHw&fui zvQt48z@67iHde9&y$>+gYd+G&=R7 zS64`-$lw2vPKdUoaXADtcwSAWzD3n?qOM*aNYytQF;Hg~jchWL~;2)Vzp{24gA;$58a6xsbQ zdnasRmo?blIFE1{; z_jz%nYvRi1%h)Z9mG`H7ZJ+JQT0SjDH}ei2hPQ7;yZEKOSma2@g|#RVuq%{0vqzeL zz7kdnW=g-EZQZ=@=C#kGOPJuG;-#QMa+PaFxp$Z_ZK318Y!*V=Wjp5~L#|EYI6U?P zdL0E|!&dV1JUsEGPQ-*^a>vENB3JG+-S6=vE>gLclM~H?oEY$01x?DKq2RzvJn)SK z!_8+M`(wU;lz~s*dZdcrS7zXE;J;+R*wDsU!otwf?xzyi#C2hQ-~dV<1lmMR&SXd znq`qEhz@2SJdO@Q5-TrwCtW7WNnhi2y)-5`7l=pg*4Oi1+gGk%$U4Hl;vjn%WGwOV zA{eE1O;=6+Ey|A+N=ny?wGMS1YvC#ZO}<__rYkPUTZ6C?+nB*}btNbj>qp)EAA{EI zgdeGcIhNH5`DQajV|Zsu&8mFG2d>?R;%5+nkNU$b`BR+_I^6QkoOm1@?I;i#fW#vp z_c-C@S~FuVm_$c5K|D6WK}ZATWhwWD&+6-|;+L~KGxP*;tl5=$Y{K7s=RT549CnPH z6wEa;?eAU2@#nb&3*gf@DlGc-t-{!`BlZ5d(Lo*&(+{HHGCER zy`a}FgqtG<=dTVt^922szsB}XrvD`{e9r#&h)Eh&?B)Q597uOata*RyVMeb46Fu9G zl$CBobZpqQwFU{L+DiX8=a(T58VD?MH|xxK)tY{wPU#!y`evMdAc^pKz9(JrVyXp9 zyN8d-Dc7=VTS_7lcX_O%t%IqhpriiKh8*dOoP+X1%NMQ8<0IY@oNhj7QLSKp7a(7( z`cQb;f>HM++B_nIrB^JG2Sd7f3sSEcxR6fe5M=q(b^!^w4-$;HM>&#&eeWTnxqfG} z`8rZn#-GEm%=jy_&Y>8^f;Zy}4j-u)`{OiQ&_BO6B zxht@QD_o|b87G4do3Y4_{`?sup`zQtc>F-+2=VY`#&uqQ58cO*=PKggT!@pQ>Zuc? z1zP8e!?#mQ&&e^j`ET!k`WLe1OC6R`JdDL=lBmjbco#F z4g9^u_b2e@m;x8$zcl=Q2mZb7@mF9geDC%DH$i^4^LsJ;m!%!}9|L|XjsFh*y=3_d z9EA0c1