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

restore explicit list of changes to PHPExcel migration document #1546

Closed
bradbulger opened this issue Jun 24, 2020 · 6 comments
Closed

restore explicit list of changes to PHPExcel migration document #1546

bradbulger opened this issue Jun 24, 2020 · 6 comments

Comments

@bradbulger
Copy link

This is:

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

The Rector tool is fine but it is not complete. (Neither was the original migration tool.) The change from #1445 removed all the details about changed function names, short names, and so on. It would be useful to restore that information.

@stale
Copy link

stale bot commented Sep 13, 2020

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 Sep 13, 2020
@bradbulger
Copy link
Author

I don't understand how I could "debug it further"? How does a change request go stale? What am I expected to do here?

@stale stale bot removed the stale label Sep 14, 2020
@PowerKiKi
Copy link
Member

For now you can read the old doc: https://github.com/PHPOffice/PhpSpreadsheet/blob/1.12.0/docs/topics/migration-from-PHPExcel.md

What did you identify as missing in the rector tool ?

@bradbulger
Copy link
Author

Things like changing Worksheet::setSharedStyle() to Worksheet::duplicateStyle(), output types from 'Excel5' to 'Xls'. The rector tool does quite a bit but there are always changes that need to be attended to by hand.

It's been a few months since I was actively working on this, but I think that even the old document may be incomplete? I'm thinking of the styling key changes particularly. Maybe you would prefer to make a separate document for that, which seems fine.

@nschulzke
Copy link

I'd agree that it would be helpful if the old documentation could be more readily accessible.

In my case, Rector failed to update column indices. It might not have been programmed to, or it might have failed to recognize the problem due to specifics of the project I was working on, but either way I ended up only figuring out during debugging that PhpSpreadsheet changed the columns to be 1-indexed. After I fixed it, I went looking to see where I'd missed that info in the documentation and found that I had to go back to an old diff of the migration instructions to find it.

With PHP being as dynamic as it is, static analysis tools will never catch 100% of problems, especially in old code that has few type annotations. Given the age of PHPExcel, it is likely that many of the projects that still need to migrate are exactly those difficult-to-analyze codebases. It would be very helpful in those cases to have the manual changes still available to reference, if only to check Rector's work after running it.

If the maintainers are on board with it, I'd be happy to submit a pull request that reinstates the old documentation with an introduction saying something along the lines of "Rector should cover most use cases, but we've included details about the manual upgrade path below for reference."

@gotgot1995
Copy link

gotgot1995 commented Jun 15, 2021

Thanks you so much @nschulzke for highlighting this issue. I ran into a similar problem as I was trying to migrate my code using rector. I had to manually review each line of code which was calling the function setCellValueByColumnAndRow() and increment all my indices +1 in order to make it work.

It would be great to specify in the API doc that indices now start from 1 and not from 0.

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

No branches or pull requests

5 participants