[4.x] Upgrade PHPOffice to v5

[IonBazan]

Please take note of our contributing guidelines: https://docs.laravel-excel.com/3.1/getting-started/contributing.html
Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.

1️⃣ Why should it be added? What are the benefits of this change?

Many users raised the need to upgrade to PHPOffice/PHPSpreadsheet v2 or v4 for extended support from upstream library and compatibility with future PHP versions. See #4263

2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.

CI changes, adjusted other dependencies and replaced legacy model factories with Laravel 8+ syntax.

3️⃣ Does it include tests, if possible?

Yes

4️⃣ Any drawbacks? Possible breaking changes?

Breaking changes:

• Drop PHP 7.4, 8.0 support
• Drop Laravel 5.8, 6 and 7 support
• Drop support for PHPSpreadsheet v1
• Add types to classes implementing PHPSpreadsheet interfaces
• PHPSpreadsheet v2 seem to not support ValueBinder when reading values - only when storing values
• PHPSpreadsheet v2 changed the logic when reading specific sheets only, breaking WithMultipleSheetsTest error handling. Temporarily removing setLoadSheetsOnly call from the Reader for now. See Option to Create Blank Sheet If LoadSheetsOnly Doesn't Find Any PHPOffice/PhpSpreadsheet#4618

Above broken features' tests were temporarily marked as skipped, hence WIP tag.

5️⃣ Mark the following tasks as done:

• Checked the codebase to ensure that your feature doesn't already exist.
• Take note of the contributing guidelines.
• Checked the pull requests to ensure that another person hasn't already submitted a fix.
• Added tests to ensure against regression.

6️⃣ Thanks for contributing! 🙌

[patrickbrouwers]

Thanks for the PR, this seems to be the first actual right attempt of updating phpoffice that actually implemented all the breaking changes.

As this is going to be in the next major release in which I'm planning to drop support for all unsupported PHP and Laravel versions, you can't drop them from the PR as well.

I don't see a need in supporting v2 and v3, we can just bump it straight away to v4

What's the problem with the loadSheetsOnly? Did you find a breaking change note about this?

What things are still WIP?

[IonBazan]

Thanks for the review, @patrickbrouwers. There is nothing else left besides loading the sheet by incorrect name, removing broken test and bumping dependency requirements. I’ll apply your suggestions and target v4 soon.

[IonBazan]

Kindly help to create 4.0 branch and target this PR there 🙏🏻
It's also worth considering to add property types and any missing return types, since we are adding breaking changes and dropping old PHP versions.

[patrickbrouwers]

I'll look into it after my holiday.
For now let's not add property and return types in this PR. It's okay in a seperate PR so we can see the impact.

[IonBazan]

Hi @patrickbrouwers I hope you had a great holiday and was wondering if you had time to look into this again. Please let me know if you wish to adjust anything with above implementation.

[patrickbrouwers]

Hey @IonBazan, I currently don't have to time to look into this. As this is a big change that I really want to test carefully before tagging, I need to have the time to spend more on it than the few minutes I have right now :( I can't promise when atm.

[mikebrandl]

V5 was just released...

[oleibman]

I am a maintainer for PhpSpreadsheet. Please let me know if there is any way I can assist in this effort. In particular, we cannot revert to the old behavior when loadSheetsOnly doesn't match any sheets because that would be a new breaking change, but we can add an option to the reader to create a new sheet if none were read in (this matches the old behavior). Would that be helpful for you?

Mention was made above that valueBinder was not supported for read. Readers and Spreadsheets have supported ValueBinder since PhpSpreadsheet 3.4.

[patrickbrouwers]

That sounds good, thx!

[oleibman]

I have created PR 4618 for PhpSpreadsheet (there is already a link to it above). Typical usage is documented in the commit message. I plan to merge it to master early next week, which will, I hope, give you a chance to see if it meets your needs before I do that. I am not planning a new release for PhpSpreadsheet until just before or just after Php8.5 is released, but, if you need it sooner, let me know. Also, please let me know if there's anything else you'd like to see which would help move this PR forward.

[patrickbrouwers]

Thanks @oleibman !

[IonBazan]

Thank you @oleibman - I have tried to use the branch you provided and restored the changes in Reader but tests are still failing at the same location. Providing incorrect sheet name throws an exception.

[oleibman]

Thank you @oleibman - I have tried to use the branch you provided and restored the changes in Reader but tests are still failing at the same location. Providing incorrect sheet name throws an exception.

I had to add this as a compatibility option in order to avoid introducing a new breaking change in PhpSpreadsheet. You need to add the following to your test:

if (method_exists($reader, 'setCreateBlankSheetIfNoneRead')) {
    $reader->setCreateBlankSheetIfNoneRead(true);
}

Once you do that, your test should succeed for both PhpSpreadsheet 1 and PhpSpreadsheet 5.

[IonBazan]

Thanks, I must have missed that! It seems to be working fine now 👍🏻

[oleibman]

@IonBazan Thank you for the confirmation. My PR is now moved to dev-master, and will be part of a formal release some time this week.

[oleibman]

PhpSpreadsheet 5.1.0, including the compatibility option described above, is now released. (Also 3.10.1 and 2.4.1 just in case.)

[IonBazan]

@patrickbrouwers I've bumped minimal version requirement to v5.1 and added testing under PHP 8.5.
I saw deprecation warnings when running under 8.5 and upon checking it locally, they seem to be caused by:
https://github.com/orchestral/testbench-core/blob/af8d6de7a747fdeaf600863c9df7eef86898d045/laravel/config/database.php#L63

PDO::MYSQL_ATTR_SSL_CA is deprecated since PHP 8.5 and Pdo\Mysql::ATTR_SSL_CA should be used instead but I don't know if we can do anything about this.

[joostdebruijn]

@IonBazan I'm following just following this conversation, expectantly waiting for a merge. Thanks for your work so far!

I was looking into the constant you mentioned, but it's a difficult one to solve because the new constant Pdo\Mysql::ATTR_SSL_CA has been introduced since PHP 8.4 but Laravel (and thus Testbench) supports down to PHP 8.2. The best solution would be to include the constant in the PHP 8.4 polyfill of Symfony so that it can be used safely, but I'm not sure if the Symfony-maintainers are open for that. I'm willing to prepare a PR for that this weekend.

It's up to @patrickbrouwers if this is a show stopper for this PR. In my opinion it is not as this deprecation is only in the test suite and PHP 8.5 is still some months away.

[patrickbrouwers]

Don't worry about the deprecation message. This is a message that will only appear the 8.5 testsuite. On future laravel/testbench updates this deprecation will be fixed.

[patrickbrouwers]

As this looks good now, I've merged it in the 4.x branch. I'll need a bit more time to prepare the actual 4.0 release

[mikebrandl]

@patrickbrouwers Any news on that 4.X release?

[patrickbrouwers]

I'm unfortunalty short on time atm, but you can of course always use the 4.x-dev in composer.