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

Rewrite "extends Object" #71

Closed
sunnysideup opened this issue May 4, 2018 · 15 comments

Comments

Projects
None yet
9 participants
@sunnysideup
Copy link

commented May 4, 2018

Sorry for all the issues... I hope it is not too annoying.

I was wondering if some of the standard replacements could be added using some basic regex ... e.g.

https://docs.silverstripe.org/en/4/changelogs/4.0.0/#object-replace

replace:


class AnyClass extends Object

with:

use SilverStripe\Core\Extensible;
use SilverStripe\Core\Injector\Injectable;
use SilverStripe\Core\Config\Configurable;
class AnyClass
{
    use Extensible;
    use Injectable;
    use Configurable;
}

Pull Requests

@robbieaverill

This comment has been minimized.

Copy link
Member

commented May 4, 2018

The example you give with extends Object should not necessarily be replaced with the traits in your example. While they will provide the full set of functionality that Object provided, one of the drivers of splitting up this functionality is that you should use the traits that your code actually needs, rather than getting more than it needs.

It is feasible to suggest that the upgrader might be able to analyse a class and say whether it has $this->extend() calls (in which case apply Extensible), Config::inst()->get(AnyClass::class) (or variations of) calls (in which case apply Configurable and upgrade to use $this->config() or static::config(), and scan a full project to find any implementations of AnyClass::create() or AnyClass::singleton() in which case apply Injectable - this would be a fair amount of work though =)

We're using an AST parser rather than regex BTW.

@sunnysideup

This comment has been minimized.

Copy link
Author

commented May 4, 2018

Thank you for your reply Robbie.

I hear you on the "only apply the traits you need", but I would see these upgrade tasks more as "simplest way" forward rather than the "most correct" way forward. The upgrade tasks would be optional (run if you deem it useful) and people can review the outcome using -vvv and then choose to enhance / refine where needed.

Great about the AST parser also.

@robbieaverill

This comment has been minimized.

Copy link
Member

commented May 4, 2018

True - what about if it also added a PHPDoc block like /** @todo determine which of these traits are actually required */ so you could go back to it?

@sunnysideup

This comment has been minimized.

Copy link
Author

commented May 4, 2018

To do docblock is perfect.

@chillu

This comment has been minimized.

Copy link
Member

commented May 6, 2018

Agree with @sunnysideup on making the upgrade a good baseline. And it looks like extending object is common enough to warrant some automation.

@chillu chillu changed the title adding a bunch of standard replacements Rewrite "extends Object" May 6, 2018

@chillu

This comment has been minimized.

Copy link
Member

commented May 6, 2018

I've narrowed the scope of this issue to "extends object", removing the other use case @sunnysideup has mentioned (which I think is a bit less valueable overall)

$baseFilePath = BASE_PATH . '/composer.json';

to

$baseFilePath = Director::baseFolder() . '/composer.json';
@sunnysideup

This comment has been minimized.

Copy link
Author

commented May 6, 2018

Can anyone give me any hints on how to write a straight replacement - as detailed in the first comment? I'd love to write it.

Am I right to look at: https://github.com/silverstripe/silverstripe-upgrader/blob/master/src/UpgradeRule/PHP/RenameTranslateKeys.php as a template for a simple replacement class? Do I need to manually add it or will it run automatically because it extends PHPUpgradeRule. I have no idea how this module works, what AST is (is sounds cool, but never heard of it), etc... and there does not seem any documentation. Of course I can find out, but any help to get me up to speed would be appreciated.

@maxime-rainville

This comment has been minimized.

Copy link
Contributor

commented May 7, 2018

@sunnysideup

The best way to do this would probably to write a new NodeVistor.

You might be able to get away and add it SilverStripe\Upgrader\UpgradeRule\PHP\RenameClasses.

Otherwise, you'll need to create a new PHPUpgradeRule to reference your new node visitor and add it to SilverStripe\Upgrader\Console\UpgradeCommand::getRules().

If you are really keen on trying to do this, I'd be happy to give you a hand. We can schedule a quick call to go over the job together.

@tractorcow

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2018

A simple straight replacement is substitute extends Object with extends ViewableData if you want a shortcut. ;)

@chillu

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

Reducing impact after feedback from team

@chillu chillu added impact/medium and removed impact/high labels Jun 6, 2018

@sminnee

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

A simple straight replacement is substitute extends Object with extends ViewableData if you want a shortcut. ;)

This would quite seriously change the meaning of your code. Adding the 3 traits would be much better.

    use Extensible;
    use Injectable;
    use Configurable;
@TomasVotruba

This comment has been minimized.

Copy link

commented Jul 31, 2018

I'm starting to looking into feature that could Rector handle based #128

I've added support for these change just a few moments ago:

# rector.yml
services:
    Rector\Rector\Class_\ParentClassToTraitsRector:
        $parentClassToTraits:
            'Object':
                - 'SilverStripe\Core\Extensible'
                - 'SilverStripe\Core\Injector\Injectable'
                - 'SilverStripe\Core\Config\Configurable'

and

vendor/bin/rector process rector --config rector.yml

will result in

-class AnyClass extends Object
+class AnyClass
 {
+    use SilverStripe\Core\Extensible;
+    use SilverStripe\Core\Injector\Injectable;
+    use SilverStripe\Core\Config\Configurable;
 }
@dhensby

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

@TomasVotruba awesome; so how do we integrate this with our tool so we don't have to ask people to install 2 tools to upgrade their code; it'd be nice to just have a passthrough to rector.

Is there any nice way to extend or add commands to rector? Maybe we could do something like map our own internal rules to rector and call the rector internals via our tool?

@TomasVotruba

This comment has been minimized.

Copy link

commented Jul 31, 2018

@dhensby There are 2 paths to do this.

One is to register Rector services as your own, maintain compatibilites and basically take care of each new BC break that happens. I needed something similar (and you might too) with coding standard tool. I needed to run rector bin file with --with-style optino to invoke conding standard tool. I could do the same as above - register coding standard services as my own, maintain compatibility etc.

But after few hours I realized it's a waste of time 🗑


So instead I used thin intergrace via console option and Symfony\Process component:
rectorphp/rector#512

Easy to use, to maintain and to extend or switch if needed 🚀


What do you think?

@dhensby

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

Ok, that would be interesting to see how we would have to build rector into our PHAR...

I also see that in the PR you link to you are making an assumption that the binary is available to execute via a path, that would be interesting to test if it were all bundled into a PHAR.

I'd like to see some way to just pull in the rector lib, supply the config and run the command through our PHP code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.