Hi there,

this is my first try at the "optimize use" (-statements) from the roadmap. I had to extend the Namescanner to collect informations about the parent, because I had to differentiate between PhpNames that are in use statements and are just somewhere in the (class-)file. I wasn't quite sure to extend it here, or create another name scanner function or collector. Thats why I added a parent property to the PhpName and had to adjust the tests.

While contributing some questions came up, that I can't find a guideline for:

• when do i need to prefix get*** for getters an when not? (PhpClass::getDeclaredNamespaceLine vs PhpName::fullyQualifiedName())
• do you write "constant" === $variable like in Symfony or does it matter at all?
• should file and line/line-range in objects of the model domain always be optional in constructor?
• i had to adjust the behat test after creating it, because of the diff @@ @@ notation. Because of this it's a little bit tricky to write expected patches for behat. Is it possible to work around it a little nicer?
• is there a guideline for commit messages? (I think i screwed it up)

Best regards Philipp

This PR provides a new mechanism for generating the patches. Please read below

Benefits

• More flexibility and resolves the problems I have mentioned before (i.e. fixes the problem I was have working on this https://github.com/QafooLabs/php-refactoring-browser/issues/50)
• Neater patch generation as all changes are taken into account at the point of creating the patch.
• Allows more complex refactorings as it would now be possible to re-analyse the partially refactored code for further refactoring rather than relying on code analysis just from the original file content. I think this will be very useful as the refactorings are added to and developped.

Implementation

The patch is generated by phpspec/php-diff which is the library phpspec uses to produce diffs https://github.com/phpspec/php-diff

This is done via a patch building library which I have created for this purpose and put into a seperate repository as I it could be useful in other project also. https://github.com/tomphp/PatchBuilder

Testing

One thing I which I should say is that I have had to modify the behat features to get it to pass, the reason for this is that behat is testing the actual patch output which had been changed (and optimised) in this version.

I think a better solution would be to test the results of the patches, i.e. generated the patches and hand them to the patch command then check the result. I'm not sure however if this would be possible using the virtual file system and therefore make require using the real file system.

While I'm 90% sure everything that I have done is correct, it might first be work updating the test suite with the new method i just mentioned against the existing code then running this pr against that test suite. Thoughts?

Tidying Up

This PR currently has all the left over classes from the old patch system lying around, I will tidy this up before expecting a merge.

What next?

I will compile and personally use this modded version for a while and see that it's working properly. If it is I think it would be great if we could go ahead with this version as it will allow progress with feature development.

Got the the buffer reloading. Next I guess it's to try and get it working without going to the console and requesting pressing enter.

I've also just realised there's a bug where it doesn't work properly if the file being refactorred isn't in the cwd, I'll see if I can work this out tomorrow. Maybe need to work out -p command for patch.

Hi,

I am using CodeLite as my PHP IDE. There is one limitation in that IDE and that is Method renaming is not supported.

I've raised that issue there. Following is the reference link:

https://github.com/eranif/codelite/issues/1725

When can we expect Method renaming feature in this library

Packages looks like abandoned, I guess because of TokenReflection - yep, I got to replace it too in ApiGen. Is that so?

I wonder, what do you instead? How were you satisfied with this solution?

I'm into building such refactor tool so I look for experience.

One of the recent changes introduced that fix-class-names changes use statments to be relative even if the were validly absolute before, see http://files.schlitt.info/tmp/diff_issues.patch.

To reproduce this diff do the following:

1. Checkout https://github.com/Qafoo/changetrack revision 60a94df
2. Move src/main/Qafoo/ChangeTrack/Analyzer/Vcs/GitCheckout.php -> src/main/Qafoo/ChangeTrack/Analyzer/Checkout/GitCheckout.php
3. Execute refactor fix-class-names src/ test/ > diff_issues.patch

Btw. the intended changes do not occur in the diff at all.

One of the recent changes introduced that refactorings might contain many empty diffs in addition to the relevant ones, see http://files.schlitt.info/tmp/diff_issues.patch.

To reproduce this diff do the following:

1. Checkout https://github.com/Qafoo/changetrack revision 60a94df5c2d761f6af6ceb3ae9e0dd979f17a40a
2. Move src/main/Qafoo/ChangeTrack/Analyzer/Vcs/GitCheckout.php -> src/main/Qafoo/ChangeTrack/Analyzer/Checkout/GitCheckout.php
3. Execute refactor fix-class-names src/ test/ > diff_issues.patch

Btw. the intended changes do not occur in the diff at all.

This is not meant to be a complete implementation of VIM bindings, I just did this so I could start using php-refactor-browser in VIM right now and I thought I'd share it in case others want to try too.

This PR does the following:

• Requires PHPUnit rather than assuming it is installed globally
• Locks Symfony component versions to 2.*
• Locks Behat to version 2.*
• Updates Behat's FeatureContext to use local PHPUnit assert function
• Locks tomphp/patch-builder to version 0.1.* (I'm planning some work on this which may cause BC breaks locking will avoid being affected).

Extract method was messing up the extracted method if the code being extracted was indented more than 4 spaces.

Also the method call to the extracted method was not indented correctly.

This PR fixes both these issues.

Unit tests & acceptance tests are included.

PS: For some reason 2 scenarios are failing the the Fix Class Names feature but there were failing before I started working so I'm assuming that they either are currently broken in the repo or something in my environment?

Using the optimize-use command on test/features/bootstrap/FeatureContext.php (known project) yields an import for the class that is declared in the file:

$ refactor optimize-use test/features/bootstrap/FeatureContext.php 
--- a/test/features/bootstrap/FeatureContext.php
+++ b/test/features/bootstrap/FeatureContext.php
@@ -10,15 +10,26 @@
 use Qafoo\ChangeTrack\Analyzer;
 use Qafoo\ChangeTrack\Calculator;
 use Qafoo\ChangeTrack\Change;
+use FeatureContext;

…

-class FeatureContext extends BehatContext
+class FeatureContext extends BehatContext

…

made this fork a while ago and the refactor.phar works afaik. won't be working on this any more but thought it might be useful for others.

This PR looks much more involved and developed but I haven't tested it out

    function test()
    {
        $userHelper = [1,2];
        unset($options[0]);
    }

Refactoring the secound line in the function will fail to take account for the unsetting changing $options and it needing to be retuned or passed by reference.

    function test()
    {
        $options = [1,2];
        $this->asd($options);
    }

    private function asd($options)
    {
        unset($options[0]);
    }

expected

    function test()
    {
        $options = [1,2];
        $options = $this->asd($options);
    }

    private function asd($options)
    {
        unset($options[0]);
        return $options;
    }

Allowing stdin input has many uses and is, in my opinion essential.

I'm not familiar with the codebase, but I came up with these changes to support stdin.

Example of use: $ cat home.php | php refactor.phar extract-method - 7-7 myNewFunction (obviously this isn't a great example of a use case)

Replacing the filename parameter with - will tell refactor.phar to use stdin as the file contents instead of a specific file.

As most commands have a dependency on the File class, it will create a tmpfile() with the stdin contents, and then create a File object from the temporary file. I've added a static File::createFromContents() function to facilitate this.

Use cases are numerous, but my specific use case/reason for implementation is for the vim-php-refactoring plugin (i've also forked to support this change). Which has multiple restrictions thrust upon it due to php-refactoring-browser not supporting stdin input.

Example: :%!php refactor.phar extract-method - 7-7 myNewFunction | patch --silent --ouput=- --dir="/tmp"

This allows on-the-fly refactoring of modified vim buffers, without worrying about loosing unsaved buffer changes, or directly modifying and having the reload the file on disk. It makes the whole experience much more seamless.

I realise that this is designed primarily for use within classes however sometimes you can't/don't have 100% of your functions as methods in a class, when working in Wordpress for example.

Having the rename-local-variable functionality working for standard global space functions as well as methods would be a nice feature. Currently it returns an error:

$ php refactor.phar rename-local-variable test.php 136 oldname newname

[QafooLabs\Refactoring\Domain\Model\RefactoringException]  
  The range 136-136 is not inside one single method.         

rename-local-variable file line name new-name

This is a task that is quite annoying and that you have to do quite often:

The syntax could be:

php refactor.phar create-setter <file> <line> php refactor.phar create-getter <file> <line>

and maybe (but not absolutely necessary) :

php refactor.phar create-getter-setter <file> <line>