Migration tool replaces any instances of PHPExcel with the filepath

[elisimmonds]

This is:

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

What is the expected behavior?

All variables containing $PHPExcel are kept the same

What is the current behavior?

The substring 'PHPExcel is replaced with the filepath to the framework.

What are the steps to reproduce?

Create a file with a variable (mine is $objPHPExcel) and then run the migration tool.

<?php

require __DIR__ . '/vendor/autoload.php';

// Create new Spreadsheet object
$spreadsheet = new \PhpOffice\PhpSpreadsheet\Spreadsheet();

$objPHPExcel // used to be this
$obj\PhpOffice\PhpSpreadsheet\Spreadsheet->setActiveSheetIndex(4); // turns into this.

Which versions of PhpSpreadsheet and PHP are affected?

I am running 1.3

[blunket]

This problem did affect me directly as well, while writing a bash script. Coincidentally I had precisely the same variable name as shown in the image above :)

I solved the problem by performing a find-replace on that variable name prior to running the migrator like so:

ack -Q '$objPHPExcel' -l | xargs perl -pi -E 's/\$objPHPExcel/\$objPHPSpreadsheet/g'

...hopefully that helps someone, but that's not an ideal solution 😄 I'd love to work on this soon. I expect I'll need to use PHP's built in tokenizer here. Seems exciting 🙂

[blunket]

@PowerKiKi -- I'd like to know your thoughts on the best way to approach this.

Consider the following code sample...

<?php
class AwesomePHPExcelFormatter {
    public $objPHPExcel  = null;
    const $ILovePHPExcel = 'PHPExcel Rocks';
    public function __construct(){
        require_once($_SERVER["DOCUMENT_ROOT"] . "/lib/PHPExcel/1.7.3/Classes/PHPExcel.php");
        $this->SetStyleDefault();
        $this->objPHPExcel = new PHPExcel();
        $this->objPHPExcel = new \PHPExcel();
        $myPHPExcel = new \namespaced\PHPExcel();
    }
    public function doRandomPHPExcelFunctions() {
    }
}

Currently, the migrator will replace every single instance of 'PHPExcel' with \PhpOffice\PhpSpreadsheet\Spreadsheet which is not what we want.

The intended result would be as follows:

<?php
class AwesomePHPExcelFormatter {
    public $objPHPExcel  = null;
    const $ILovePHPExcel = 'PHPExcel Rocks';
    public function __construct(){
        require_once($_SERVER["DOCUMENT_ROOT"] . "/lib/PHPExcel/1.7.3/Classes/PHPExcel.php");
        $this->SetStyleDefault();
        $this->objPHPExcel = new PhpOffice\PhpSpreadsheet\Spreadsheet();
        $this->objPHPExcel = new \PhpOffice\PhpSpreadsheet\Spreadsheet();
        $myPHPExcel = new \namespaced\PhpOffice\PhpSpreadsheet\Spreadsheet();
    }
    public function doRandomPHPExcelFunctions() {
    }
}

By using the PHP tokenizer, I can filter out any variable names, and I can filter out T_CONSTANT_ENCAPSED_STRING (covering the require in my sample code above). However, this isn't really sufficient.

On line 2 in the sample code, the tokenizer gives the following (I've formatted this for readability):

T_CLASS     : class
T_WHITESPACE:  
T_STRING    : AwesomePHPExcelFormatter

... and on line 8 (where it should replace) it gives:

T_VARIABLE       : $this
T_OBJECT_OPERATOR: ->
T_STRING         : objPHPExcel
T_WHITESPACE     :
=
T_WHITESPACE:  
T_NEW       : new
T_WHITESPACE:  
T_STRING    : PHPExcel

notice that in this case, the tokenizer returns T_STRING in $this->objPHPExcel instead of T_VARIABLE, and it returns T_STRING again in new PHPExcel ...

So basically, I think we have to add a parser to get this right 🙁 We can't have it replacing in variable names, class variables, function names, class method names, and probably even more cases I'm not thinking about.

What are your thoughts? Should we add a parser? I'm willing to take a stab at it depending on what you think.

[PowerKiKi]

While we should expect a lot of users using this tool, it is meant as a one shot usage. And it is meant to be removed from the project in the future (maybe in the next or next next major version). So improving it would be great, but we shouldn't spend too much time on it either. Given the temporary nature of the tool, I feel that a parser would be overkill. I was imagining more like regexp-ing our way around variables. Any word that start with `$` should be excluded from replacement. Or something along those lines...

[blunket]

Okay, fair enough. and good point.

How does this regex look to you?

(?<!(\$|->))\bPHPExcel\b

It matches the first two lines of the following, but none of the last five:

PHPExcel
\PHPExcel
$PHPExcel
objPHPExcel
$objPHPExcel
$this->objPHPExcel
$this->PHPExcel

Am I missing any cases? If not I'll have a PR for you soon!

[blunket]

I actually had to un-group the OR in my lookbehind above.

This regex seems to work fairly well!

In my example above, it only fails in the places within the quoted strings. Should I try to account for that too? Or is this good enough for now?