From afd070a75618dcdbcc4d627f8f563c4e487ff920 Mon Sep 17 00:00:00 2001 From: oleibman Date: Fri, 3 Jan 2020 15:10:41 -0800 Subject: [PATCH] Handle ConditionalStyle NumberFormat When Reading Xlsx File (#1296) * Handle ConditionalStyle NumberFormat When Reading Xlsx File ReadStyle in Reader/Xlsx/Styles.php expects numberFormat to be a string. However, when reading conditional style in Xlsx file, NumberFormat is actually a SimpleXMLElement, so is not handled correctly. While testing this change, it turned out that reader always expects that there is a "SharedString" portion of the XML, which is not true for spreadsheets with no string data, which causes a run-time message. Likewise, when conditional number format is not one of the built-in formats, a run-time message is issued because 'isset' is used to determine existence rather than 'array_key_exists'. The new workbook added to the testing data demonstrates both those problems (prior to the code changes). * Move Comment to Resolve Conflict Github reports conflict involving placement of one comment statement. * Respond to Scrutinizer Style Suggestion Change detection for empty SimpleXMLElement. --- src/PhpSpreadsheet/Reader/Xlsx/Styles.php | 18 +++++++- src/PhpSpreadsheet/Style/NumberFormat.php | 2 +- .../Reader/CondNumFmtTest.php | 39 ++++++++++++++++++ tests/data/Reader/XLSX/condfmtnum.xlsx | Bin 0 -> 9649 bytes 4 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Reader/CondNumFmtTest.php create mode 100644 tests/data/Reader/XLSX/condfmtnum.xlsx diff --git a/src/PhpSpreadsheet/Reader/Xlsx/Styles.php b/src/PhpSpreadsheet/Reader/Xlsx/Styles.php index c98780b0..40106258 100644 --- a/src/PhpSpreadsheet/Reader/Xlsx/Styles.php +++ b/src/PhpSpreadsheet/Reader/Xlsx/Styles.php @@ -8,6 +8,7 @@ use PhpOffice\PhpSpreadsheet\Style\Borders; use PhpOffice\PhpSpreadsheet\Style\Color; use PhpOffice\PhpSpreadsheet\Style\Fill; use PhpOffice\PhpSpreadsheet\Style\Font; +use PhpOffice\PhpSpreadsheet\Style\NumberFormat; use PhpOffice\PhpSpreadsheet\Style\Protection; use PhpOffice\PhpSpreadsheet\Style\Style; @@ -71,6 +72,17 @@ class Styles extends BaseParserClass } } + private static function readNumberFormat(NumberFormat $numfmtStyle, \SimpleXMLElement $numfmtStyleXml) + { + if ($numfmtStyleXml->count() === 0) { + return; + } + $numfmt = $numfmtStyleXml->attributes(); + if ($numfmt->count() > 0 && isset($numfmt['formatCode'])) { + $numfmtStyle->setFormatCode((string) $numfmt['formatCode']); + } + } + private static function readFillStyle(Fill $fillStyle, \SimpleXMLElement $fillStyleXml) { if ($fillStyleXml->gradientFill) { @@ -149,7 +161,11 @@ class Styles extends BaseParserClass private function readStyle(Style $docStyle, $style) { - $docStyle->getNumberFormat()->setFormatCode($style->numFmt); + if ($style->numFmt instanceof \SimpleXMLElement) { + self::readNumberFormat($docStyle->getNumberFormat(), $style->numFmt); + } else { + $docStyle->getNumberFormat()->setFormatCode($style->numFmt); + } if (isset($style->font)) { self::readFontStyle($docStyle->getFont(), $style->font); diff --git a/src/PhpSpreadsheet/Style/NumberFormat.php b/src/PhpSpreadsheet/Style/NumberFormat.php index 7efa759e..5ea30b88 100644 --- a/src/PhpSpreadsheet/Style/NumberFormat.php +++ b/src/PhpSpreadsheet/Style/NumberFormat.php @@ -367,7 +367,7 @@ class NumberFormat extends Supervisor self::fillBuiltInFormatCodes(); // Lookup format code - if (isset(self::$flippedBuiltInFormats[$formatCode])) { + if (array_key_exists($formatCode, self::$flippedBuiltInFormats)) { return self::$flippedBuiltInFormats[$formatCode]; } diff --git a/tests/PhpSpreadsheetTests/Reader/CondNumFmtTest.php b/tests/PhpSpreadsheetTests/Reader/CondNumFmtTest.php new file mode 100644 index 00000000..ca69d23b --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/CondNumFmtTest.php @@ -0,0 +1,39 @@ +load($filename); + + $worksheet = $spreadsheet->getActiveSheet(); + // NumberFormat explicitly set in following conditional style + $conditionalStyle = $worksheet->getConditionalStyles('A1:A3'); + self::assertNotEmpty($conditionalStyle); + $conditionalRule = $conditionalStyle[0]; + $conditions = $conditionalRule->getConditions(); + self::assertNotEmpty($conditions); + self::assertEquals(Conditional::CONDITION_EXPRESSION, $conditionalRule->getConditionType()); + self::assertEquals('MONTH(A1)=10', $conditions[0]); + $numfmt = $conditionalRule->getStyle()->getNumberFormat()->getFormatCode(); + self::assertEquals('yyyy/mm/dd', $numfmt); + // NumberFormat not set in following conditional style + $conditionalStyle = $worksheet->getConditionalStyles('B1'); + self::assertNotEmpty($conditionalStyle); + $conditionalRule = $conditionalStyle[0]; + $conditions = $conditionalRule->getConditions(); + self::assertNotEmpty($conditions); + self::assertEquals(Conditional::CONDITION_EXPRESSION, $conditionalRule->getConditionType()); + self::assertEquals('AND(B1>=2000,B1<3000)', $conditions[0]); + $numfmt = $conditionalRule->getStyle()->getNumberFormat()->getFormatCode(); + self::assertEquals('', $numfmt); + } +} diff --git a/tests/data/Reader/XLSX/condfmtnum.xlsx b/tests/data/Reader/XLSX/condfmtnum.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..9254f87f09c2ae6833bc3f532cbb0b7d43909e1d GIT binary patch literal 9649 zcmeHN^;cEv*4}g@-AIQ>2}n1HlynPhy1OL>Bsa~bBqS9nH{IPKlF}t0-L(OMZ+q@J z*K@djf5E-$hxLv%#+q}i^{)BM`OIf76$J!D0st}q6#xLx0FdIn&yC;!0AEA^03Uz~ z|5D1)!Oh&k4XE+T$=ua|-OJvNIu{Y1IU4{EyZ?X3fAJ2KfCn8qIB}j`Bwam-uhlf! z%q?Pp2Z<)LscoU8dP%ZU%pGGA+_vDK=x7MPF-l}E-^lXWD@V!Gw5?>2=2=Y21)fSH zmmSa6=|1vjq`Iu^ivy1jc%u~PAViKH-a2sVqO?U7QzqxqpmV61Oo|sn4&f+bCq!!^ zq1?k}DwF!3#YNA(l`D>8+EkS?LrJg~v$Zf3X09Ux95WQ7@n?1{48vGL(A(e2U2u? z#$nB?$bNy?@i`A!MS0}uN4;ToZLQlY{&(JD97-Vl1|I8(NG;OZ6ThH$jCqQmZlj~! zE)xz(sk%gvxkNtI#+eNR;=d9dejUt`R*tVv>K#Q{8^>F5xhi|wxGfC9k>-Lp4ojss zF&(Qm3bMOd>IRFpC2#GM+``~@cZUE_`3EqoH96@{VbP`t!yN_;m_QeEJ68_&pXdJp z^1m2`e|z1r(t~B58qoJuh?@O`v6PlalItrJ;~ozU=M9 zFX;hO$?vzY#KHv>cgNBQ`&>;dXUcs0>}bv|$u!li1gs$@SuTRKuYhKDZ70&{t@t;- zY)Tn@YP39fP_8e}dl|CN0`#hQjs`Pdb@8Ef7WW*9UZ6dX6aoOyj{*QZ zfbEQz9fzl*i>-;HqwUYiRiZiPFvpMM2h6^sKFq|GGq6KbMW3sHz|GRiR&FT-2E{Xj z!s&|$2!&9u4k+bo5_!65)60w#8N&pHxm_*XuKcGahBHT+HSB{n(IjH5Ts8YV60-x! zxP+f8IeNW3)T^eIxX68e@MZDFy+QNpT;}6T6u_*gMA8$3-3C9yh|mkBB78O-`2unW zqmO(JOV;h67Q7Cg>OoYrh0sBw7pUd_SfMNmX&{YtDLOswKr2;Ogwtzzj~lqsejvh> zlb?_qzxE3h9KMR=@BWC-loES4(zUGp)r04R$oPe@mT>&RZEe_mfm_|0(L>eSn$}8y z*-Y8i!*_ToW0o}2%~9<;alYChA!}DJvw}{(3iMc=6*>}zfG8OWy*w=w)J5{qAUeL# zVI>7!(2iUK_CbSkQ+ZlZy!y#v35Kf|6LZ?;qG`q7I7*JG?>!bNdrOx$H%L6qGX4Vm6C_3F2 zQ~z`oYWeom79H@WK@K3b*J-7*e~56P>ubD@dk~1uaj8zVtnCk_mGz zgBlh2*|50<{aEIEAM5K0QNepq;gx?Reoke#0Spb^3v-|5Ew_DCbCZXUGle4FGxZl96q0ft)6Nj^F?7mAgTiHX^xylToe#d{FD-ZU&R z=0WyA1`O%T!Vx;j;akPDNf3{{GJ8T7AKO2R0TGKO$+~{n`B=<+$EZeWYOw8lZ_PFI z&dNKlaJAE?tD>ZiSNrWo?csSn!Fs|4bzwiv@-=vuE0rSxh2SR2V)_Hl$U4`lbDV`C zXRuCL@NRsGd#kVMU|Qqqcu+drz^K-@x|qGxJLnA5 zKMd`w$s2Y0j!@5I?Mo4zPx2D8=b@rPUH=Jj)SXba@Q%i{p49x*m+YcP^v8P+OFYd5 zo{q6}0`;%OldWZmRgxmuB0^g;7@oHcLO+WfC1izYT~Rn2f^4z7MJQYRgq@z#7xz|l z^D7gP3SX}s6aOwg%mHcZ=&*c@1Ix!Gun+%dK6bS-H+OU8_+xqUGcji*y;Pp#BO3 z($AG*jp&MH4Mq| zP3E4_dEG?eH|wZ?t7dAF)1FSH019lOm5Frs&_s#W@r#=o-A#Y8y5K<|e}oz~X^kpP zcLhmo^C*3r65iz5NpxkkKd(&a&_SBf1n0;hXT`ce%JC?fVJ8?v+JIb#HYfDcGy^_u z4?g6pm<<+5(CbQdFHwg{3wMbCRc3S2k8kP|cOLf;DfE{hG++dnPjtcZA$aq{+q1sb;z3DjZvQY7lTVmfK9N|aSW^iZpq?j$=eWOnq? z6Z#0SJ{>bPxq_UXkT+d=dx|d{Q_0r!>@1~Rm3)WMazlJ&Q25rkw;%ggvBrHAW5?Rv z*V9V48ZlZ`H9p<`0iU2Kctz`0`PF`*n4tl$X6P#g=vYADIZ#kIBM8}{p7%lpxC$^S zm@!}=;hMv1$g*D^7<*w`kUm6&XI7`7&r-*1G9Qd}9$Jhj=ODTys-GFvJU)Fw_aQUn zq$KcDFinC8zb`2!iU+EPVboyqtXN0LZp%KNFtSg|Zm5>8uaHQZ7ZuTW;ZWQ88&p%{ zVFZ$_+t@G$T?lzDQPYSlq+~xyRHn$9sTRYOPGL^MZw_xvoV*IC;3ZE2F5hDZ%URDp z*fbh!wtUZmyN2cs@uxJ=mp3O^K_-wnXHlE79zSfXWxd4u9e&!0aaxpM*y*AE*^U2! zA2%y=dvlII&VN8>SNAiBoR6TD_(}}Z)$_;G)mVnbei(y`QIIbR;ALTU3rkGNPta>Bo};Z26gae3fOvQMl%44wo3LKNRNP`McoS_oMbs zBV0<`xt0AZ$4ue%y@YI zR+kJn8^pC7M59`a((A~&QV!X7T=j}ZzB;3>$6M_-W7Md@dOig}ghKt^vz;5g)|#;o za8^7)N$+c>1$UAh;*7CWt|oIDwGT80i55qI29@=cMX@1M*%|?mP%6xZ)4FJU&IWSXij+aMKunq zcvnqbQHPdqT`y{%OBC=LUX5PvTYopE@(5~09f(xwR10%u71UuACI#q`KURvM0{=!w@$)ZMFLJYFFS!1_0=p2 z4k!%j>id(iz`L6p;zeNV?cw$j-_``vQAbznkITW7*1IFYGsw4jLXH-{g^79$X>@B zg-#DrlW?o;Oas+XJ$+2tCz*1jdDAEl5>LV9qPK&GhfZ0F3yCN75seWe@wWTy@+E+a z4eo61Z)!8q5cBU?k$4gB`P;|TTg#=QD2p4V!q=xnxV|YuB2M>lJ_U!5Gf24@F{rUu zNtuKXc9>JYnLYlzX#c6R&~{S9*Nn~>_8gr4;UCv~2Cdjtchv->pFE{(0hz6FxdgO7 zXDs8Om-dc)baG|N`bOcqVAvU%L@PSxD)#k5gD&0#1((gSdJqYByokqiw2H9ntYSi6 zmp>ym&9ivVU^!yn*cl;d5h)6liYN26Xf+)-7PNIpYc_AeOyO7N!<6MsQEAOo#BMzfQFV8h#yUun7;PD1ali!8>X+%PTl?bB4&c`zN@sxzY)6W46)2&SHt z`9fPpq~e)|7dKlKD;sy~qULA$ zY;;j>mC>AIoI%7ZtU{lYuXTGG(!wUVl>PxS$Ju>siv zlCXG2DRS)}Yrn=3)mwcl{+KG=8OKei&x@;_&N_5~L^Vr^DsV-;%;~nMdx^4yeuko> znJPNA89{J^4>w;&hvP9yOvGzYu*>%6CxPh@!ua(|&u>NpR#Q!pV*=n9Izf<%M{cTF zR>#wbvY}Ds$5bTBGjH2-cG!heixAG6l^6J^CI?)Rt-o8o?`GMaa7)|@6I#P6K9-uO zU@368QDSo`vt_Tx#+!U4-?TE}2SRA)WP3Nn^+LBvsWY;&QgyD1n-vvPd4d;e?=HhH za#Y47)|WY?=Pu5=BcBPq8kldIv_SC;6!vxs;QyeusUqo^LdSeHM&1YJH3H2i>PDk3 zH3=}W-T6PVLs^iYW&@KMx8i67tL5O<6o+__Xbs*#^tatv9xNNhtLkHq?_9y@8;?Bm zu>RCL#lF9~&h|M37X(rS4iQ(cTzzZNpYe4*z@^T-t{%wNwfC?Iv8WbdMr>Bf8`=FT z7-CrZ@&LpkM z;PP|ZMnW3Dn?CwIN4H*rsH$06n;hHo`~%Fq7Y9Ug5@Y;X_;U3faK;Qfckr|yFTFQ& z^#)_~T2_PR5_>yjzZS>(6ksi&Ih~~rSw+Y6&*yJi4X1nURQl0>ir#m(bp)hr)~rAE z+22oKMJwfyef@(spPtUyoA7$H3D;T%)8|`^sQ&kD>c`eT8au%i;u}_y#%rbsJ(rky zZ38lVaUZ5dQo#bG$tkajCkc=eP(SMXXD*ueaG<%2=7@yU z^~yy1IXVk(8}!wup+$0U`7yhS*HElM-w!Vk(9DT+?+-?xWGCeZ06E`%iFxbfGH1JrL}K>ANvTgaYEp&bS-Nf-d>{spn0+QFY4 z;=ja%Uwz^qQo@_q_Q(!Sf@c>9w^Cj{iC;r7rPTRo>ohkIOs-~;=ThP>2yT2TDa_l} z*PXts^QK=5e8k{uqc-#i3Mvz`EwA9V{rKrhB^^ z<3}azkk>jad;H~%oIsqBXs)b4-?P2r*CrA>p66M%e%Ofb?S}?W&S7cfKX>5E^7E<= z*xR!b0su_^+5smQYuM1uMcv%Z&Dz1zmE+#^6Lu48hh2@&^7ASrBHZPBoA0p+=G|2D zA3JYiCVpJdOw0F>AA3X3UFZD7O${}wKTcY@AP7n(Dx1P?a1PhtLDP-ml(+GodpbEM zYFP%lzuvg?MMQw_BF>kW)>k4Dk#z5|OHbm)Im)6@!&)m`(BHL?N z(>Mi*T(51<^cGZpJ$mr9IuK!ceU|ScJ08|dmKm|Q5 zfXpc3t%&`obW~66?4rH2-rJgi^F1~QjUbh;-Iw~-EZ1?nmPGeMv>vVK0o(`^g4f@o z3nv(#E3eZwNd~$rqG|D=yHufozIR{h_B>sW4n!HZ;w*zF_e!V$?@-uhjuV&;)>5ZM zH%Yy6ULfhu5POUDHLA4|YneWV1A^F#=!)strV=adCXOyjdlvT5=k+p)OH`N2j4aqXKZ+C?8&RGsX8~9pyt)SyTzeN+aBF;glWrQ za-*^*-F_G>I+ACMX#7AORPr*Wg*DiQ0s1W1VFw#4U2UegN({)9*g zXw^qg#2I2XpG>ES16n}`Veky zcp1lqx&{S~`Z$l2_~!y01zm^v4AYdan0zQ3%{t;k)XzFYEPZ7gy=+kpr(TI_%zZU0 zyhKvk5yhGiapbzDFG3A>s+Y2_5YT|MnQJN8!fXTLPCVbD|^DhvjP6U zA6NeRTz{><`He+I;hzrvIbi?G@aLKa!^Gdl?e`7u4;_D*wjlpo^8RZCdEfY-6O>=3 z002AsAIAUBJmtQh`%3#SPfD2oeuzI=)h|AE-^+cy@t2nv*fo5PG5vCY z1sf3jqkp<@dY=jYGIb)m{|3M1h4&raCsw~4x{&|j@E)!DEzP>`^q*z=mrDTP6(s=h oA4U7V`9IU_-_4;^e>49x>8dCo!6NAA{~j2CH5f0brTuyJe|7iA5&!@I literal 0 HcmV?d00001