Skip to content
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

IOFactory::load emits errors instead of throwing exceptions #167

Closed
1 of 3 tasks
Finesse opened this issue Jun 1, 2017 · 12 comments
Closed
1 of 3 tasks

IOFactory::load emits errors instead of throwing exceptions #167

Finesse opened this issue Jun 1, 2017 · 12 comments

Comments

@Finesse
Copy link

Finesse commented Jun 1, 2017

This is:

What is the expected behavior?

PhpOffice\PhpSpreadsheet\IOFactory::load throws an PhpOffice\PhpSpreadsheet\Reader\Exception exception when it gets a file which it can't read. According to the documentation, exception must be thrown in case of problems.

Or at least I can suppress errors by placing the @ operator before calling the load method.

What is the current behavior?

When I run the script via a web server, the script just stops (browser says that the connection has been broken by the server).

When I run the script via a console, IOFactory::load emits errors which are not catchable by try-catch:

  • PHP 7.0: zend_mm_heap corrupted
  • PHP 7.1: Segmentation fault: 11

Placing the @ operator before calling IOFactory::load changes nothing.

What are the steps to reproduce?

  1. Download this file (file.zip) and put it near the script below.
  2. Run the script:
    <?php
    
    require __DIR__ . '/vendor/autoload.php';
    
    try {
        $spreadsheet = \PhpOffice\PhpSpreadsheet\IOFactory::load(__DIR__.'/file.zip');
        echo 'Your file is good.';
    } catch (\Exception $e) {
        echo 'Your file is wrong!';
    }

Which versions of PhpSpreadsheet and PHP are affected?

PhpSpreadsheet 1.0.0-beta1 – 1.1.0. PHP 7.0.15, 7.1.1 (didn't test it in other versions). I run it on macOS 10.13.3.

@Finesse
Copy link
Author

Finesse commented Jun 1, 2017

I've found that the code on the line 363 in the file src/PhpSpreadsheet/Shared/StringHelper.php makes the script exit without a chance to handle error:

$value = @iconv('UTF-8', 'UTF-8', $value);

As I know iconv is OS dependent so the bug may be OS dependant. I use macOS 10.12.5. The PHP_OS constant value is Darwin.

@Finesse
Copy link
Author

Finesse commented Jun 1, 2017

The script exit can be solved by removing this block of code:

if (self::getIsIconvEnabled()) {
    $value = @iconv('UTF-8', 'UTF-8', $value);

    return $value;
}

But it doesn't remove the warnings from the question. I don't know if it is a good solution.

@PowerKiKi
Copy link
Member

Could you investigate whether one of those answers helped in your case ?

@Finesse
Copy link
Author

Finesse commented Jul 31, 2017

@PowerKiKi, unfortunately I can't reproduce the simplexml_load_string errors. But I still face a problem when I run the «how to reproduce» example above: the script just stops, browser says that the connection has been broken by the server.

Update. I've just succeed to reproduce the simplexml_load_string errors by removing the @ symbol from the StringHelper::sanitizeUTF8 code. Then I added libxml_use_internal_errors(true); to the code, and got the same error from which I started (the script just stops, browser says that the connection has been broken by the server). Adding the LIBXML_NOWARNING option to simplexml_load_string doesn't help either.

@stale
Copy link

stale bot commented Dec 4, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added stale and removed stale labels Dec 4, 2017
@Finesse
Copy link
Author

Finesse commented Dec 5, 2017

The issue is still actual for 1.0.0-beta2, but the error message changed.

@stale
Copy link

stale bot commented Feb 3, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added stale and removed stale labels Feb 3, 2018
@Finesse
Copy link
Author

Finesse commented Feb 3, 2018

This issue is still actual for version 1.1.0 😕

@PowerKiKi
Copy link
Member

I actually cannot reproduce the zend_mm_heap or Segmentation fault. This is most likely a bug in PHP itself or one of its extension. And there is not much we can do.

What could be done though, is to prevent the CSV reader to accept any file. We should ideally only accept CSV files. Maybe via mimetype detection if that proves to be reliable enough on CSV files. But that should be tested.

@Finesse
Copy link
Author

Finesse commented Feb 5, 2018

@PowerKiKi I don't have any extension, I use pure PHPs from MAMP. I will make a Travis CI demo soon.

@Finesse
Copy link
Author

Finesse commented Mar 14, 2018

@PowerKiKi Unfortunately, Travis CI can't test PHP on macOS (i tried).

I run the tests on my mac and got some errors:

  • PhpOffice\PhpSpreadsheetTests\Calculation::testIMEXP#0:
    Mismatched Real part: 187004.112739 != 187004.112739
    Failed asserting that false is true.
     ~/tests/PhpSpreadsheetTests/Calculation/EngineeringTest.php:230
    
  • PhpOffice\PhpSpreadsheetTests\Calculation::testIMSQRT#1:
    Mismatched Real part: 58223.706512 != 58223.706512
    Failed asserting that false is true.
     ~/tests/PhpSpreadsheetTests/Calculation/EngineeringTest.php:344
    
  • PhpOffice\PhpSpreadsheetTests\Writer\Ods\Content::testWriteSpreadsheet:
    Failed asserting that two DOM documents are equal. <Click to see difference>
    
     ~/tests/PhpSpreadsheetTests/Writer/Ods/ContentTest.php:96
    
    The difference:
    2018-03-14 21 20 36

@Finesse
Copy link
Author

Finesse commented Mar 14, 2018

Tested the issue in the version 1.2.0: it is fixed 🍻

Dfred pushed a commit to Dfred/PhpSpreadsheet that referenced this issue Nov 20, 2018
CSV reader used to accept any file without any kind of check. That made
users incorrectly believe that things were ok, even though there is no
way for CSV reader to read anything else that plain text files.

Fixes PHPOffice#167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants