-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Address Some Chart Problems #3771
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix PHPOffice#3767. The basic problem in that issue was user error, but it exposed a couple of problems in code which are being addressed. When a spreadsheet with charts is loaded without the includeCharts option and then saved, a corrupt spreadsheet is created. I believe this has been the case for quite some time. Nothing in the test suite covers this scenario. It is, in fact, a difficult thing to test, since the problem is exposed only when the file is opened through Excel. The specific problem is that a rels file generated by the output writer continues to refer to the drawing file which described the chart, but that file is not included (and not needed) in the output spreadsheet. The resolution is kludgey. The information that the file will not be needed is not available when the rels file is written. But, when it comes time to write the drawing file, it is known whether the rels file included it. So, if nothing else has caused the file to be generated, it is written out as a basically empty xml file after all. This solves the problem, but I will continue to search for a less kludgey solution. This solution is, at least, testable; if a different solution is applied later on, the test being introduced here is likely to break so a new one will be needed. When the provided spreadsheet is loaded with the includeCharts option and then saved, an error is exposed in processing the Chart Title caption when it doesn't exist. The change to Writer/Xlsx/Chart is simple and clearly justifiable. What is peculiar is that the error does not arise with release 1.29, but does arise with master. It is not at all clear to me what has changed since the release to expose the error - the code in question certainly hasn't changed. It is difficult to isolate changes because of the extensive number of changes following on the elimination of Php7.4 as a supported platform. The provided spreadsheet is unusual in at least two senses. When opened in Excel, it will show a clearly default value for the chart title, namely 'Chart Title'. I cannot find anything in the xml corresponding to that text. Since I have no idea why Excel is using that title, I will not try to duplicate its behavior, so that loading and saving the provided spreadsheet will omit the chart title. I will continue to investigate. The other sense in which it is unusual is that it includes some style files in the same directory as the chart. I doubt that PhpSpreadsheet looks at these. The styling after load and save seems to mostly match the original, although there is at least one color in the graph which does not match. I imagine it would be pretty complicated to formally support these files.
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Oct 18, 2023
Fix PHPOffice#3770. A rels file points to a non-existent theme file in the spreadsheet. In other similar cases (e.g. PR PHPOffice#3771), Excel opens such a spreadsheet, but with an error pop-up. Not so with this file; it just opens the spreadsheet without the pop-up. PhpSpreadsheet will now account for this unusual situation as well.
11 tasks
oleibman
added a commit
that referenced
this pull request
Oct 26, 2023
* Theme File Missing But Referenced in Spreadsheet Fix #3770. A rels file points to a non-existent theme file in the spreadsheet. In other similar cases (e.g. PR #3771), Excel opens such a spreadsheet, but with an error pop-up. Not so with this file; it just opens the spreadsheet without the pop-up. PhpSpreadsheet will now account for this unusual situation as well. * Update CHANGELOG.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #3767. The basic problem in that issue was user error, but it exposed a couple of problems in code which are being addressed.
When a spreadsheet with charts is loaded without the includeCharts option and then saved, a corrupt spreadsheet is created. I believe this has been the case for quite some time. Nothing in the test suite covers this scenario. It is, in fact, a difficult thing to test, since the problem is exposed only when the file is opened through Excel. The specific problem is that a rels file generated by the output writer continues to refer to the drawing file which described the chart, but that file is not included (and not needed) in the output spreadsheet. The resolution is kludgey. The information that the file will not be needed is not available when the rels file is written. But, when it comes time to write the drawing file, it is known whether the rels file included it. So, if nothing else has caused the file to be generated, it is written out as a basically empty xml file after all. This solves the problem, but I will continue to search for a less kludgey solution. This solution is, at least, testable; if a different solution is applied later on, the test being introduced here is likely to break so a new one will be needed.
When the provided spreadsheet is loaded with the includeCharts option and then saved, an error is exposed in processing the Chart Title caption when it doesn't exist. The change to Writer/Xlsx/Chart is simple and clearly justifiable. What is peculiar is that the error does not arise with release 1.29, but does arise with master. It is not at all clear to me what has changed since the release to expose the error - the code in question certainly hasn't changed. It is difficult to isolate changes because of the extensive number of changes following on the elimination of Php7.4 as a supported platform.
The provided spreadsheet is unusual in at least two senses. When opened in Excel, it will show a clearly default value for the chart title, namely 'Chart Title'. I cannot find anything in the xml corresponding to that text. Since I have no idea why Excel is using that title, I will not try to duplicate its behavior, so that loading and saving the provided spreadsheet will omit the chart title. I will continue to investigate.
The other sense in which it is unusual is that it includes some style files in the same directory as the chart. I doubt that PhpSpreadsheet looks at these. The styling after load and save seems to mostly match the original, although there is at least one color in the graph which does not match. I imagine it would be pretty complicated to formally support these files.
This is:
Checklist:
Why this change is needed?
Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.