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

Error: Typed property PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::$parent must not be accessed before initialization #3993

Closed
pan85 opened this issue Apr 18, 2024 · 6 comments · Fixed by #4057

Comments

@pan85
Copy link

pan85 commented Apr 18, 2024

What is the expected behavior?

private ?Spreadsheet $parent = null;

What is the current behavior?

private ?Spreadsheet $parent;

Which versions of PhpSpreadsheet and PHP are affected?

2.0.0

@oleibman
Copy link
Collaborator

Do you have code which demonstrates that the current code fails as described?

@marc-mabe
Copy link

I have the same issue happening on destruct a mocked Worksheet.

object(TestStub_Worksheet_87f22e33)#748 (39) {
  ["parent":"PhpOffice\PhpSpreadsheet\Worksheet\Worksheet":private]=>
  uninitialized(?PhpOffice\PhpSpreadsheet\Spreadsheet)
...
Error: Typed property PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::$parent must not be accessed before initialization in /app/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Worksheet/Worksheet.php:380
Stack trace:
#0 /app/src/XXXHandler.php(54): PhpOffice\PhpSpreadsheet\Worksheet\Worksheet->__destruct()
#1 /app/tests/XXXHandlerTest.php(105): XXXHandler->__invoke(Object(XXXCommand))

For me it gets fixed by initializing the parent property on class level with null as it's nullable anyway:

BEFORE:  private ?Spreadsheet $parent;
AFTER:   private ?Spreadsheet $parent = null;

@oleibman
Copy link
Collaborator

@marc-mabe Can you share more of your code? Based on your description, I tried:

        $body = $this->getMockBuilder(Worksheet::class)
                    ->disableOriginalConstructor()
                    ->getMock();
        $body = null;sleep(5);
        gc_collect_cycles();

It did not throw an exception/error.

Even if the mock method were to work in simulating the problem, I'm just having trouble understanding how it can happen "natively". The first statement in the constructor initializes the property that is supposedly uninitialized. There is no unset statement for the property. How does it ever get to a state where the property is uninitialized?

@marc-mabe
Copy link

Hi @oleibman,

I'm also unsure why exactly it happens but I'm not actively creating the mock but a sheet creator service and testing a specific function (creating the worksheet) to be called.

It looks more like this (shorten as much as possible - not tested):

class SheetCreator {
    public function createInfoSheet(Spreadsheet $workbook, $info): Worksheet
    {
        $worksheet = $workbook->createSheet();
        $worksheet->setTitle($info);
        // ...
        return $worksheet;
    } 
}

class MyHandler {
    public function __construct(
        private readonly CreateTableHandler $createTableHandler,
        private readonly SheetCreator $infoSheetCreator,
    ) {}

    public function __handle(MyCommand $command):void {
        $table = $this->createTableHandler->__invoke($command->whatever);
        $workbook = TableToPhpSpreadsheet::createSpreadsheet($table);
        $this->infoSheetCreator->createInfoSheet($workbook, $info);
        // ...
        $xlsx = new Xlsx($workbook);
        $xlsx->save($file);
    }
}

class MyHandlerTest {
    public function testHandler(): void
    {
        $tableCreatorHandlerMock = $this->createMock(CreateTableHandler::class);
        $creatorMock = $this->createMock(SheetCreator::class);
        $handler  = new MyHandler($creatorMock);

        // ...
        $table = new MyTable();
        $tableCreatorHandlerMock
            ->expects(static::exactly(1))
            ->method('__invoke')
            ->with($whatever)
            ->willReturn($table);

         $creatorMock
            ->expects(static::exactly(1))
            ->method('createInfoSheet')
            ->with(static::isInstanceOf(Spreadsheet::class), $info);

        // ...
        $handler->__invoke($command);
        // ....
    }
}

@oleibman
Copy link
Collaborator

I apologize, I am not able to get your code into a state where I can work with it, e.g. CreateTableHandler class is not defined, ditto for MyTable, MyHandler constructor requires 2 arguments but you supply only 1, no idea what command should be on the last line with code, and more. Are you able to flesh it out a bit to something that I can run?

@oleibman
Copy link
Collaborator

oleibman commented Jun 1, 2024

Good news! We currently use Phpunit 9 for our unit tests, but, now that we support only Php 8.1+, Dependabot recommended a switch to Phpunit 10. This requires some changes, mostly to test members. But ... test Reader/Xlsx/AutoFilterTest, which indeed uses mocking, failed with "must not be accessed before initialization"! And the suggested change eliminates that error. So expect a fix when I'm ready to push the Phpunit change.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Jun 2, 2024
Dependabot suggested some changes this month which required an unusual effort to implement successfully. With the elimination of Php 8.0 as a supported environment, it became possible to use Phpunit 10 rather than 9. Among other considerations, the configuration file for Phpunit is changed. I preserved the Phpunit 9 version under a different name. Aside from the configuration change, several other changes needed to be made to accommodate the change:
- Fix PHPOffice#3993. A peculiar problem indeed. One of the reporters said it had something to do with mocking, but I couldn't duplicate it. But Phpunit 10 revealed the problem in one test (Reader/Xlsx/AutoFilterTest), and that was sufficient for me to apply the trivial source code change to Worksheet/Worksheet.
- Some extra stringency on data providers required extra work in Calculation/CalculationFunctionListTest.
- There was a misplaced label in Calculation/ParseFormulaTest. Likewise in the data member CellGetRangeBoundaries.
- More stringency required changes to data members Shared/Trend/ExponentialBestFit and Shared/Trend/LinearBestFit.
- Issue3982Test testLoadAllRows seemed to go into a memory-acquiring loop in 10 that was not evident in 9. This particular test uses a lot of memory by design, but was included only to establish a base level for the other tests in that member. I feel it is acceptable to skip it for 10.

Phpstan errors with a new release are not unusual. One of the problems this time around was, however, unusual - it is fixed when Phpstan runs under Php8.3, but not for earlier Php releases. Our tools currently use Php 8.1. It is on my to-do list to get Phpstan and other test tools running under 8.3 before 8.1 goes EOL. There are also some configuration file changes needed for Phpstan.

Php-cs-fixer has been taking an increasingly long time to run. They've added an experimental option to permit it to run its checks in parallel. I've changed its configuration to use that option.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants