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

[PHPExcel] migration to [PhpSpreadsheet] #6171

Closed
kjeanson opened this issue Apr 20, 2021 · 14 comments
Closed

[PHPExcel] migration to [PhpSpreadsheet] #6171

kjeanson opened this issue Apr 20, 2021 · 14 comments

Comments

@kjeanson
Copy link

kjeanson commented Apr 20, 2021

Hi,

I try to migrate PHPExcel to PhpSpreadsheet.

Everything is ok (composer require / rector.php config file)

I have a /tests/PHPExcelTest.php file for PHPUnit wich is containing a PHPExcel sample :
$objPHPExcel = new PHPExcel(); ...

When i run vendor/bin/rector process tests/ i have a progressbar 52/52 and a green message [OK] Rector is done! but nothing append. My "new PHPExcel" is not replace by PhpSpreadsheet.

Do you have any idea ?

Thanks.

@sabbelasichon
Copy link
Contributor

I think this is a bug related to the problem that the class PHPExcel does not exists in your project and is not replaced.
In order to check this try to create a stub class PHPExcel and load the stubs via composer´s autoloading classmap mechanism.
https://github.com/rectorphp/rector/blob/main/composer.json#L103

@sabbelasichon
Copy link
Contributor

sabbelasichon commented Apr 20, 2021

If you can verify, that after adding the class it is working, i am gonna create a failing PR for that.

@sabbelasichon
Copy link
Contributor

@TomasVotruba That´s definitely related to this issue here: sabbelasichon/typo3-rector#2161.

If you are saying, that should be possible without autoloading this would be great. If not or maybe as in between step, we could you use this neat but very smart library here: https://github.com/TYPO3/class-alias-loader.

So if we provide all the old classes we are coming from in a ClassAliasMap the problem is gone. I am using exactly this approach in my projects. But this still requires autoloading. What do you think?

@TomasVotruba
Copy link
Member

TomasVotruba commented Apr 20, 2021

If you are saying, that should be possible without autoloading this would be great.

That's the goal here, as there is no type check needed. The replacement is basically string for string. Current implementation of class renamer is wrong and requires type check.

@kjeanson
Copy link
Author

@sabbelasichon thanks for your quick reply

I added a classmap on my composer.json
"classmap": [
"vendor/phpoffice/phpexcel/Classes"
]

now when i run vendor/bin/rector process tests it seems to load PHPExcel library because i have the following PHP Deprecated messages
Deprecated: Array and string offset access syntax with curly braces is deprecated in /vendor/phpoffice/phpexcel/Classes/PHPExcel/Shared/String.php on line 542
but still no replacement...

I tried to add composer´s autoloading files "vendor/phpoffice/phpexcel/Classes/PHPExcel.php", but same issue...

@sabbelasichon
Copy link
Contributor

@TomasVotruba Do you think it could help to add stubs for this? For me in typo3-rector it works perfectly now. I thinks it is somehow related. See: sabbelasichon/typo3-rector@c924f8c#diff-5d6f21616131ebbf56ab692f8d9f7cef73b1c7167c5a33e8fd7bc8262cb3a2c5R4

@TomasVotruba
Copy link
Member

@sabbelasichon I haven't looked into this yet.
Failing PR with this use case would help me to look into this closer.

@sabbelasichon
Copy link
Contributor

@TomasVotruba I am not able to create a failing test for that. It works perfectly as you said.

@kjeanson Never asked before, but what version of rector are you using? In current master it works perfectly without having any related classes in place.

@TomasVotruba
Copy link
Member

@kjeanson We'll need a reproducible repository on Github to verify.

@kjeanson
Copy link
Author

kjeanson commented Apr 28, 2021

@TomasVotruba this is my repo :
https://github.com/Groupe-ESC-Troyes/RectorPHPExcel/
Do composer install
and vendor/bin/rector process tests
It will show you 4/4 [============================] 100% and [OK] Rector is done!

N.B : my rector version is 0.10.11

@TomasVotruba
Copy link
Member

Thanks! I'm looking into it

@TomasVotruba
Copy link
Member

When running with --debug:

vendor/bin/rector process tests

It seems the problem is with protected method:

image

This file is included by composer, so it's invoked:

{
    "autoload": {
		"files": [
			"services/Intranet/Autoloader.php"
	    ],		
	}
}

Making it public fixes public call and works 👍

image

@TomasVotruba
Copy link
Member

There is one more problem if the whole fixture is included. I'm looking into it

@kjeanson
Copy link
Author

@TomasVotruba Thanks a lot ! It finally works when i comment the throw new PHPExcel_Writer_Exception line 8 in files :
vendor/phpoffice/phpexcel/Classes/PHPExcel/Writer/PDF/DomPDF.php
vendor/phpoffice/phpexcel/Classes/PHPExcel/Writer/PDF/mPDF.php
vendor/phpoffice/phpexcel/Classes/PHPExcel/Writer/PDF/tcPDF.php

TomasVotruba added a commit that referenced this issue Jul 22, 2024
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

No branches or pull requests

3 participants