silverstripe / silverstripe-upgrader

A tool to help upgrade your code to handle API changes in packages you used.
BSD 3-Clause "New" or "Revised" License
39 stars 21 forks source link

Certain rules should be ignored by default #23

Open robbieaverill opened 7 years ago

robbieaverill commented 7 years ago

Similar to #16

For example:

$fields->dataFieldByName('Email')->addExtraClass('form-group');

The upgrader will currently replace with this:

$fields->dataFieldByName('SilverStripe\\Core\\Email\\Email')->//...

If easy enough to implement, it's be nice to implement an extensible set of rules, for example containing something like:

Do not upgrade strings that are arguments of [dataFieldByName, fieldByName]

Better yet, ignore arguments that are in the context of any method on a FieldList instance (wishful thinking??)


Another example:

$field = new UploadField('Image', 'Image');

# upgrades to:
$field = new UploadField('SilverStripe\\Assets\\Image', 'SilverStripe\\Assets\\Image');

It should be ignored, because the arguments for UploadField are not class names (if we could add UploadField::__construct to this ignore context that'd be awesome)

dhensby commented 6 years ago

has any progress been made on this? it seems like a fairly important issue to me

robbieaverill commented 6 years ago

Things have moved a lot since then. @maxime-rainville might know whether this type of thing is supported now with more complex rules and using phpstan

maxime-rainville commented 6 years ago

Was /** @skipUpgrade */ available when this issue was raise?

Those namespace substitution are applied by the upgrade command. This command doesn't actually parse the PHP file, so it wouldn't know that that UploadFiled is a FormField. I don't think they're would be a nice elegant way of saying "never apply the namespace substitution to a string if it's in the constructor of a class if it's a descendant of FormField".

robbieaverill commented 6 years ago

Yeah it was available then. The problem (?) with it is that it requires you to firstly know what the upgrader does and how it works in order to annotate parts of your code that should be ignored, and also to have done that before running it. It's a little disparate to the idea of a magic tool that does all the things for you.

If the tools at hand aren't analysing argument types then it'd be unlikely we could do this automatically, but perhaps we could still say "don't upgrade argument 1 of addFieldToTab calls, maybe?

maxime-rainville commented 6 years ago

We could try adding a few specific rules such as "don't apply NS substitution if the method is named dataFieldByName", but those are likely to be a bit arbitrary.

Also, it's never going to be bullet proof. e.g.:

$fieldName = 'Email';
$fields->dataFieldByName($fieldName)->addExtraClass('form-group');

My instincts would be to do it only for the most used methods.