-
Notifications
You must be signed in to change notification settings - Fork 2
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
SPIKE Have another go at identifiy a tool that could help with automating lots of changes between majors #244
Comments
I don't think we need this card
I wouldn't want to automate this. This is really matter of slowly adding strongly typing to existing methods and then repeatedly running it locally to see if it breaks and later running it through a comprehensive CI suite to see what breaks, because some methods that call stuff on DataObject will now be passing the wrong type. If we attempt to automate this then everything just breaks at once and just be really hard to isolate what caused the breakage. I've previously looked a doing mass strongly typing where I automatically added strong typing to methods based on the docbock types, and it went hilariously badly. The docblock types are just plain wrong on many, many, many methods. Automation simply isn't feasible.
Personally I'd write a quick script to go through ever file in vendor/(silverstripe|symbiote|etc) and look for
We'd presumably be changing the parent class of HTTPRequest and HTTPResponse and likely keep most of the same public API, similar to what we did for Email with CMS 5 where we also switched the parent class to a different symfony class. If there's any small changes the public api then we can just manually update them, I wouldn't want automation here.
I strongly disagree with the use of rector I tried using rector for PHP 8.1 and CMS 5, it was absolutely terrible, not even close to suitable. It managed to be both overly complicated and underpowered. I'm not going to use it again. I ended up making https://github.com/emteknetnz/php81-fixer for this and it was fantastic. The heavy lifting to parse the PHP file in a tree structure is done by nikic/php-parser . From there you can write custom script to do whatever we want. That said I probably wouldn't even want to to parse PHP files for the examples listed here and even that's over-complicated / overkill / inappropriate. Something like php81-fixer might make sense for some other examples that require across the changes, though I think that should be decided on a case-by-case basis. I do not think it should be considered any sort of "documented common approach", as I don't think there should an sort of common approach as the correct tool for the job totally going to depend on what the job is |
We will aim to add more examples of possible CMS 6 cards before we pick this up. |
I feel as though that if actually do this card it should be done closer to CMS 6 release rather than now because we'll have an idea of the actual surface area of API changes, rather than now where we can only speculate. Right now I'm of the opinion there simply won't be enough API changes to warrant such a tool. For CMS 5 API changes we used https://github.com/emteknetnz/silverstripe-deprecator to handle things like generating the giant changelog of API changes between CMS 4 and CMS 5. This tool, like the php81-fixer use for PHP 8.1, uses nikic/php-parser do so the PHP class parsing. However even with that large amount of API changes, we didn't need an upgrader tool for end users to go from CMS 4 to CMS 5, and the feedback we've been given is that CMS 4 to CMS 5 was a smooth upgrade. We're likely to have even less API changes going from CMS 5 to CMS 6 so the likely scenario is that there's simply no need for an upgrader tool. Right now it just seems like we're trying to select a tool to solve an undefined problem which is just wrong IMO |
Probably not going to happen. |
CMS 6 will likely have a lot more substantial API changes than CMS 5. Before we go too far down the track of changing a bunch of API, I'm be keen to revisit Rector and how it could integrate with efforts like strongly typing more of our code or handling the transition away from deprecated API.
Objective
Note
The text was updated successfully, but these errors were encountered: