Skip to content

Conversation

@devOp1
Copy link
Contributor

@devOp1 devOp1 commented Nov 29, 2022

PhpExcel is replaced with PhpSpreadsheet to make yii2-excelexport php8.0 ready. Looking forward for a new version with this PR included.

@mikehaertl
Copy link
Collaborator

mikehaertl commented Nov 29, 2022

@devOp1 Thanks for the contribution. I currently have no time to have a closer look.

Did you test that everything still works? I'm actually surprised that there were so little changes. I always thought this would require some bigger refactoring.

In any case the README should also be updated.

Related issue: #24

@devOp1
Copy link
Contributor Author

devOp1 commented Nov 30, 2022

@mikehaertl I tested it with a large generated excel with several worksheet. For converting the code i used the rector script from https://phpspreadsheet.readthedocs.io/en/latest/topics/migration-from-PHPExcel/. I am not 100% sure if i configured composer correctly and would like to test that after merging.

@mikehaertl
Copy link
Collaborator

@devOp1 Ok, then it's probably fine. Could also go through the README and update the relevant parts (changed class names, link to PHPExcel, maybe update the example to handle styling etc.)? Then I'll merge and create a new release.

@mikehaertl
Copy link
Collaborator

@devOp1 Sorry, I forgot a "you" in the question above 😄 . Could you go through the README ...?

Otherwhise I can't promise when I will find time to do it. But I want to keep the README up to date before a release.

@devOp1
Copy link
Contributor Author

devOp1 commented Nov 30, 2022

@mikehaertl Done.

@mikehaertl mikehaertl merged commit d430c46 into codemix:master Nov 30, 2022
@mikehaertl
Copy link
Collaborator

@devOp1 Do you need a release or can you do some more testing first?

@devOp1
Copy link
Contributor Author

devOp1 commented Nov 30, 2022

I can test without a specific release. Thanks

@mikehaertl
Copy link
Collaborator

Cool. Please report back if everything works.

@devOp1
Copy link
Contributor Author

devOp1 commented Dec 5, 2022

It's working so far. But i will keep my eyes open.

@mikehaertl
Copy link
Collaborator

Ok, I think it's safe to create a release. We can still fix things in a later bugfix release.

Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants