-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate Table Cell if Row Doesn't Have Any #2516
base: master
Are you sure you want to change the base?
Conversation
Fix PHPOffice#2505. Word treats file as corrupt if a table row does not contain a cell (documentation for why this is so is included in the issue). Person reporting the issue suggests that dropping such a row from the output file is preferred. However, I think generating an empty cell instead is closer to the user's expectation. For example, as demonstrated in the unit tests added with this PR, if a table has row 1 and 3 which contain cells, but row 2 does not, the table as written to the file will have 3 rows, with the second containing an empty cell.
Hi @oleibman , tested locally and LGTM. Thank you. |
$cells = $row->getCells(); | ||
if (count($cells) === 0) { | ||
// issue 2505 - Word treats doc as corrupt if row without cell | ||
$this->writeCell($xmlWriter, new CellElement()); | ||
} else { | ||
foreach ($cells as $cell) { | ||
$this->writeCell($xmlWriter, $cell); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$cells = $row->getCells(); | |
if (count($cells) === 0) { | |
// issue 2505 - Word treats doc as corrupt if row without cell | |
$this->writeCell($xmlWriter, new CellElement()); | |
} else { | |
foreach ($cells as $cell) { | |
$this->writeCell($xmlWriter, $cell); | |
} | |
} | |
if (count($row->getCells()) === 0) { | |
// Add a cell if the row has no cells (avoid corrupt file) | |
$row->addCell(); | |
} | |
foreach ($row->getCells() as $cell) { | |
$this->writeCell($xmlWriter, $cell); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this suggestion. The way I have coded it, there is no change to the user's document object after the write. With your suggestion, there will be a new cell added to the user's document object. This seems unexpected. If the user were to write, then add a cell to the row (perhaps user is preparing multiple documents), it would now be in the row's second cell rather than the first. Given that, do you still think this should be recoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would mutate the document object when writing out a file. As a user I would expect that the document object is not modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oleibman Great job.
Added some comments.
Could you update the changelog file, too, please ?
@Progi1984 Change log updated, all your suggestions except one are incorporated; that one needs further discussion, |
Description
Fix #2505. Word treats file as corrupt if a table row does not contain a cell (documentation for why this is so is included in the issue). Person reporting the issue suggests that dropping such a row from the output file is preferred. However, I think generating an empty cell instead is closer to the user's expectation. For example, as demonstrated in the unit tests added with this PR, if a table has row 1 and 3 which contain cells, but row 2 does not, the table as written to the file will have 3 rows, with the second containing an empty cell.
Checklist:
composer run-script check --timeout=0
and no errors were reported