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

Migration faux pas #481

Closed
Synchro opened this issue Apr 30, 2018 · 4 comments
Closed

Migration faux pas #481

Synchro opened this issue Apr 30, 2018 · 4 comments

Comments

@Synchro
Copy link
Contributor

Synchro commented Apr 30, 2018

This is:

- [x] a bug report
- [ ] a feature request
- [x] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

The migration tool does a number of somewhat dumb, unexpected things.

What is the expected behavior?

As far as it can, the migration tool should make the changes necessary to move from PHPExcel to PHPSpreadsheet, but no more.

What is the current behavior?

The migration makes some changes that are inadvisable and easily avoided.

It appears to do a blanket global replacement of PHPExcel with \PhpOffice\PhpSpreadsheet\Spreadsheet, but this includes places that this change should not be made, causing breakages. For example, I had a private property that held a PHPExcel instance, and it did this:

private $PHPExcel;

became:

private $\PhpOffice\PhpSpreadsheet\Spreadsheet

Which is not even valid PHP.

The instructions for the migration suggest running it in the project root, which includes the vendor folder - so the migration is applied to the installed PHPExcel package, and also makes changes to PHPSpreadsheet package wherever it happens to mention PHPExcel.

The migration should not touch anything in a vendor folder.

What are the steps to reproduce?

You don't even need any code to demonstrate this; an empty project is enough:

composer init
composer require phpoffice/phpexcel
composer require phpoffice/phpspreadsheet
vendor/phpoffice/phpspreadsheet/bin/migrate-from-phpexcel

The output shows that it has made changes to both the PHPExcel and PHPSpreadsheet files (excerpt):

/Users/marcus/Sites/pxtest/vendor/phpoffice/phpexcel/install.txt converted
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpexcel/Classes/PHPExcel.php converted
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpexcel/Classes/PHPExcel/Autoloader.php converted
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpexcel/Classes/PHPExcel/CachedObjectStorageFactory.php converted
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpexcel/Classes/PHPExcel/Calculation.php converted
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpexcel/Classes/PHPExcel/Cell.php converted
...
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpspreadsheet/docs/topics/migration-from-PHPExcel.md converted
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpspreadsheet/samples/Basic/46_ReadHtml.php converted
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Helper/Migrator.php converted
/Users/marcus/Sites/pxtest/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Reader/Ods.php converted
/

Note that this also applies the replacements in non-PHP files, which is quite unnecessary.

Which versions of PhpSpreadsheet and PHP are affected?

I'm using PHPExcel 1.8.1 and PHPSpreadsheet 1.2.1. It probably affects others.

As far as I can see, the current blanket search and replace mechanism is extremely unlikely to produce working code, and probably causes more problems than it solves. The matches and replacements could be made much more specific, or better still, be based on an AST search rather than simple text.

@stale
Copy link

stale bot commented Jun 29, 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 the stale label Jun 29, 2018
@Synchro
Copy link
Contributor Author

Synchro commented Jun 29, 2018

I assume this is is still an issue...

@stale stale bot removed the stale label Jun 29, 2018
@PowerKiKi
Copy link
Member

Indeed nothing was done to improve the situation. A PR would be very welcome if you have time.

@blunket
Copy link
Contributor

blunket commented Jul 6, 2018

Hope my PR is sufficient. Let me know. This bug was affecting me directly also.

Dfred pushed a commit to Dfred/PhpSpreadsheet that referenced this issue Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants